[PATCH 0/3] watchdog: Add Amlogic Meson GXBB Watchdog Timer driver

[PATCH 0/3] watchdog: Add Amlogic Meson GXBB Watchdog Timer driver

[Neil Armstrong]

Adds support for the Amlogic Meson GXBB SoC Watchdog Timer.
It differs from the meson6/meson8b HW, so need for a separate driver.
The HW provides a divider capable of having a 1ms timebase thus simplifying
the counter update.
The restart call is not provided even if the HW is capable of triggering a
system reset immediately because of the PSCI firmare having such functionnality.
The watchdog is not expected to be running at boot time since there is a separate
system-level watchdog running from the SCPI co-processor, but this case would be
handle in a next driver update.

Changes since RFC version at http://lkml.kernel.org/r/1464249112-13658-1-git-send-email-narmstrong@baylibre.com :
- Remove status callback, will re-introduce it later to managed the already-running use case
- Fix registers defines indentation
- Fix space between operators
- Make all callbacks static functions
- Fix bindings with missing clocks attribute
- Do not stop/start watchdog on a timeout setup
- Fix probe device configuration

Neil Armstrong (3):
  watchdog: Add Meson GXBB Watchdog Driver
  dt-bindings: watchdog: Add Meson GXBB Watchdog bindings
  ARM64: dts: amlogic: meson-gxbb: Add watchdog node

 .../bindings/watchdog/meson-gxbb-wdt.txt           |  16 ++
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |   6 +
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/meson_gxbb_wdt.c                  | 277 +++++++++++++++++++++
 4 files changed, 300 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt
 create mode 100644 drivers/watchdog/meson_gxbb_wdt.c

-- 
2.7.0

[PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Neil Armstrong]

Add watchdog specific driver for Amlogic Meson GXBB SoC.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/watchdog/Makefile         |   1 +
 drivers/watchdog/meson_gxbb_wdt.c | 277 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 278 insertions(+)
 create mode 100644 drivers/watchdog/meson_gxbb_wdt.c

diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 9bde095..7679d93 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o
 obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
 obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
 obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
+obj-$(CONFIG_MESON_WATCHDOG) += meson_gxbb_wdt.o
 obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
 obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
 obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
new file mode 100644
index 0000000..9619c5e
--- /dev/null
+++ b/drivers/watchdog/meson_gxbb_wdt.c
@@ -0,0 +1,277 @@
+/*
+ * This file is provided under a dual BSD/GPLv2 license.  When using or
+ * redistributing this file, you may do so under either license.
+ *
+ * GPL LICENSE SUMMARY
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ * The full GNU General Public License is included in this distribution
+ * in the file called COPYING.
+ *
+ * BSD LICENSE
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ *   * Redistributions of source code must retain the above copyright
+ *     notice, this list of conditions and the following disclaimer.
+ *   * Redistributions in binary form must reproduce the above copyright
+ *     notice, this list of conditions and the following disclaimer in
+ *     the documentation and/or other materials provided with the
+ *     distribution.
+ *   * Neither the name of Intel Corporation nor the names of its
+ *     contributors may be used to endorse or promote products derived
+ *     from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/watchdog.h>
+
+#define DEFAULT_TIMEOUT	10	/* seconds */
+
+#define GXBB_WDT_CTRL_REG			0x0
+#define GXBB_WDT_CTRL1_REG			0x4
+#define GXBB_WDT_TCNT_REG			0x8
+#define GXBB_WDT_RSET_REG			0xc
+
+#define GXBB_WDT_CTRL_EE_RESET_NOW		BIT(26)
+
+#define GXBB_WDT_CTRL_CLKDIV_EN			BIT(25)
+#define GXBB_WDT_CTRL_CLK_EN			BIT(24)
+#define GXBB_WDT_CTRL_IRQ_EN			BIT(23)
+#define GXBB_WDT_CTRL_EE_RESET			BIT(21)
+#define GXBB_WDT_CTRL_XTAL_SEL			(0)
+#define GXBB_WDT_CTRL_CLK81_SEL			BIT(19)
+#define GXBB_WDT_CTRL_EN			BIT(18)
+#define GXBB_WDT_CTRL_DIV_MASK			(BIT(18) - 1)
+
+#define GXBB_WDT_CTRL1_GPIO_PULSE		BIT(17)
+#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0		BIT(16)
+#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1		(0)
+#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT		(BIT(16) - 1)
+
+#define GXBB_WDT_TCNT_SETUP_MASK		(BIT(16) - 1)
+#define GXBB_WDT_TCNT_CNT_SHIFT			16
+
+struct meson_gxbb_wdt {
+	void __iomem *reg_base;
+	struct watchdog_device wdt_dev;
+	struct clk *clk;
+};
+
+static int meson_gxbb_wdt_start(struct watchdog_device *wdt_dev)
+{
+	struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+	writel(readl(data->reg_base + GXBB_WDT_CTRL_REG) | GXBB_WDT_CTRL_EN,
+		data->reg_base + GXBB_WDT_CTRL_REG);
+
+	return 0;
+}
+
+static int meson_gxbb_wdt_stop(struct watchdog_device *wdt_dev)
+{
+	struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+	writel(readl(data->reg_base + GXBB_WDT_CTRL_REG) & ~GXBB_WDT_CTRL_EN,
+		data->reg_base + GXBB_WDT_CTRL_REG);
+
+	return 0;
+}
+
+static int meson_gxbb_wdt_ping(struct watchdog_device *wdt_dev)
+{
+	struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+	writel(0, data->reg_base + GXBB_WDT_RSET_REG);
+
+	return 0;
+}
+
+static int meson_gxbb_wdt_get_timeout(struct meson_gxbb_wdt *data)
+{
+	return (readl(data->reg_base + GXBB_WDT_TCNT_REG) &
+			GXBB_WDT_TCNT_SETUP_MASK) / 1000;
+}
+
+static int meson_gxbb_wdt_set_timeout(struct watchdog_device *wdt_dev,
+				      unsigned int timeout)
+{
+	struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+	meson_gxbb_wdt_ping(wdt_dev);
+
+	writel(timeout * 1000, data->reg_base + GXBB_WDT_TCNT_REG);
+
+	return 0;
+}
+
+static unsigned int meson_gxbb_wdt_get_timeleft(struct watchdog_device *wdt_dev)
+{
+	struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
+	unsigned long reg;
+
+	reg = readl(data->reg_base + GXBB_WDT_TCNT_REG);
+
+	return ((reg >> GXBB_WDT_TCNT_CNT_SHIFT)-
+			(reg & GXBB_WDT_TCNT_SETUP_MASK)) / 1000;
+}
+
+static const struct watchdog_ops meson_gxbb_wdt_ops = {
+	.start = meson_gxbb_wdt_start,
+	.stop = meson_gxbb_wdt_stop,
+	.ping = meson_gxbb_wdt_ping,
+	.set_timeout = meson_gxbb_wdt_set_timeout,
+	.get_timeleft = meson_gxbb_wdt_get_timeleft,
+};
+
+static const struct watchdog_info meson_gxbb_wdt_info = {
+	.identity = "meson-gxbb-wdt",
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+};
+
+static int __maybe_unused meson_gxbb_wdt_resume(struct device *dev)
+{
+	struct meson_gxbb_wdt *data = dev_get_drvdata(dev);
+
+	if (watchdog_active(&data->wdt_dev))
+		meson_gxbb_wdt_start(&data->wdt_dev);
+
+	return 0;
+}
+
+static int __maybe_unused meson_gxbb_wdt_suspend(struct device *dev)
+{
+	struct meson_gxbb_wdt *data = dev_get_drvdata(dev);
+
+	if (watchdog_active(&data->wdt_dev))
+		meson_gxbb_wdt_stop(&data->wdt_dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops meson_gxbb_wdt_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(meson_gxbb_wdt_suspend, meson_gxbb_wdt_resume)
+};
+
+static const struct of_device_id meson_gxbb_wdt_dt_ids[] = {
+	 { .compatible = "amlogic,meson-gxbb-wdt", },
+	 { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, meson_gxbb_wdt_dt_ids);
+
+static int meson_gxbb_wdt_probe(struct platform_device *pdev)
+{
+	struct meson_gxbb_wdt *data;
+	struct resource *res;
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->reg_base))
+		return PTR_ERR(data->reg_base);
+
+	data->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(data->clk))
+		return PTR_ERR(data->clk);
+
+	clk_prepare_enable(data->clk);
+
+	platform_set_drvdata(pdev, data);
+
+	data->wdt_dev.parent = &pdev->dev;
+	data->wdt_dev.info = &meson_gxbb_wdt_info;
+	data->wdt_dev.ops = &meson_gxbb_wdt_ops;
+	data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
+	data->wdt_dev.min_timeout = 1;
+	data->wdt_dev.timeout = DEFAULT_TIMEOUT;
+	watchdog_set_drvdata(&data->wdt_dev, data);
+
+	/* Setup with 1ms timebase */
+	writel(((clk_get_rate(data->clk) / 1000) & GXBB_WDT_CTRL_DIV_MASK) |
+		GXBB_WDT_CTRL_EE_RESET |
+		GXBB_WDT_CTRL_CLK_EN |
+		GXBB_WDT_CTRL_CLKDIV_EN,
+		data->reg_base + GXBB_WDT_CTRL_REG);
+
+	meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
+
+	ret = watchdog_register_device(&data->wdt_dev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int meson_gxbb_wdt_remove(struct platform_device *pdev)
+{
+	struct meson_gxbb_wdt *data = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&data->wdt_dev);
+
+	return 0;
+}
+
+static void meson_gxbb_wdt_shutdown(struct platform_device *pdev)
+{
+	struct meson_gxbb_wdt *data = platform_get_drvdata(pdev);
+
+	meson_gxbb_wdt_stop(&data->wdt_dev);
+}
+
+static struct platform_driver meson_gxbb_wdt_driver = {
+	.probe	= meson_gxbb_wdt_probe,
+	.remove	= meson_gxbb_wdt_remove,
+	.shutdown = meson_gxbb_wdt_shutdown,
+	.driver = {
+		.name = "meson-gxbb-wdt",
+		.pm = &meson_gxbb_wdt_pm_ops,
+		.of_match_table	= meson_gxbb_wdt_dt_ids,
+	},
+};
+
+module_platform_driver(meson_gxbb_wdt_driver);
+
+MODULE_ALIAS("platform:meson-gxbb-wdt");
+MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
+MODULE_DESCRIPTION("Amlogic Meson GXBB Watchdog timer driver");
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.7.0

[PATCH 3/3] ARM64: dts: amlogic: meson-gxbb: Add watchdog node

[Neil Armstrong]

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index 832815d..bcca82f 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -136,6 +136,12 @@
 				clocks = <&xtal>;
 				status = "disabled";
 			};
+
+			watchdog@98d0 {
+				compatible = "amlogic,meson-gxbb-wdt";
+				reg = <0x0 0x098d0 0x0 0x10>;
+				clocks = <&xtal>;
+			};
 		};
 
 		gic: interrupt-controller@c4301000 {
-- 
2.7.0

Re: [PATCH 0/3] watchdog: Add Amlogic Meson GXBB Watchdog Timer driver

[Carlo Caione]

On 30/05/16 15:29, Neil Armstrong wrote:
> Adds support for the Amlogic Meson GXBB SoC Watchdog Timer.
> It differs from the meson6/meson8b HW, so need for a separate driver.
> The HW provides a divider capable of having a 1ms timebase thus simplifying
> the counter update.
> The restart call is not provided even if the HW is capable of triggering a
> system reset immediately because of the PSCI firmare having such functionnality.
> The watchdog is not expected to be running at boot time since there is a separate
> system-level watchdog running from the SCPI co-processor, but this case would be
> handle in a next driver update.
> 
> Changes since RFC version at http://lkml.kernel.org/r/1464249112-13658-1-git-send-email-narmstrong@baylibre.com :
> - Remove status callback, will re-introduce it later to managed the already-running use case
> - Fix registers defines indentation
> - Fix space between operators
> - Make all callbacks static functions
> - Fix bindings with missing clocks attribute
> - Do not stop/start watchdog on a timeout setup
> - Fix probe device configuration
> 
> Neil Armstrong (3):
>   watchdog: Add Meson GXBB Watchdog Driver
>   dt-bindings: watchdog: Add Meson GXBB Watchdog bindings
>   ARM64: dts: amlogic: meson-gxbb: Add watchdog node

I'm missing [PATCH 2/3]

-- 
Carlo Caione

Re: [PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Carlo Caione]

On 30/05/16 15:29, Neil Armstrong wrote:
> Add watchdog specific driver for Amlogic Meson GXBB SoC.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> +

> +#define GXBB_WDT_CTRL1_REG			0x4
...
> +#define GXBB_WDT_CTRL_EE_RESET_NOW		BIT(26)
...
> +#define GXBB_WDT_CTRL_IRQ_EN			BIT(23)
...
> +#define GXBB_WDT_CTRL_XTAL_SEL			(0)
...
> +#define GXBB_WDT_CTRL_CLK81_SEL			BIT(19)
...
> +#define GXBB_WDT_CTRL1_GPIO_PULSE		BIT(17)
...
> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0		BIT(16)
> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1		(0)
> +#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT		(BIT(16) - 1)

nit: all these defines are not used at all in the driver. You can remove them

Thanks!


-- 
Carlo Caione

[PATCH 2/3] dt-bindings: watchdog: Add Meson GXBB Watchdog bindings

[Neil Armstrong]

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 .../devicetree/bindings/watchdog/meson-gxbb-wdt.txt      | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt b/Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt
new file mode 100644
index 0000000..c7fe36f
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt
@@ -0,0 +1,16 @@
+Meson GXBB SoCs Watchdog timer
+
+Required properties:
+
+- compatible : should be "amlogic,meson-gxbb-wdt"
+- reg : Specifies base physical address and size of the registers.
+- clocks : Should be a phandle to the Watchdog clock source, for GXBB the xtal
+	   is the default clock source.
+
+Example:
+
+wdt: watchdog@98d0 {
+	compatible = "amlogic,meson-gxbb-wdt";
+	reg = <0 0x98d0 0x0 0x10>;
+	clocks = <&xtal>;
+};
-- 
2.7.0

Re: [PATCH 0/3] watchdog: Add Amlogic Meson GXBB Watchdog Timer driver

[Neil Armstrong]

On 05/30/2016 03:45 PM, Carlo Caione wrote:
> On 30/05/16 15:29, Neil Armstrong wrote:
>> Adds support for the Amlogic Meson GXBB SoC Watchdog Timer.
>> It differs from the meson6/meson8b HW, so need for a separate driver.
>> The HW provides a divider capable of having a 1ms timebase thus simplifying
>> the counter update.
>> The restart call is not provided even if the HW is capable of triggering a
>> system reset immediately because of the PSCI firmare having such functionnality.
>> The watchdog is not expected to be running at boot time since there is a separate
>> system-level watchdog running from the SCPI co-processor, but this case would be
>> handle in a next driver update.
>>
>> Changes since RFC version at http://lkml.kernel.org/r/1464249112-13658-1-git-send-email-narmstrong@baylibre.com :
>> - Remove status callback, will re-introduce it later to managed the already-running use case
>> - Fix registers defines indentation
>> - Fix space between operators
>> - Make all callbacks static functions
>> - Fix bindings with missing clocks attribute
>> - Do not stop/start watchdog on a timeout setup
>> - Fix probe device configuration
>>
>> Neil Armstrong (3):
>>   watchdog: Add Meson GXBB Watchdog Driver
>>   dt-bindings: watchdog: Add Meson GXBB Watchdog bindings
>>   ARM64: dts: amlogic: meson-gxbb: Add watchdog node
> 
> I'm missing [PATCH 2/3]
> 
Sorry, fixed here :

Re: [PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Guenter Roeck]

On 05/30/2016 06:29 AM, Neil Armstrong wrote:
> Add watchdog specific driver for Amlogic Meson GXBB SoC.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>   drivers/watchdog/Makefile         |   1 +
>   drivers/watchdog/meson_gxbb_wdt.c | 277 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 278 insertions(+)
>   create mode 100644 drivers/watchdog/meson_gxbb_wdt.c
>
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 9bde095..7679d93 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o
>   obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
>   obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
>   obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
> +obj-$(CONFIG_MESON_WATCHDOG) += meson_gxbb_wdt.o

No extra Kconfig entry ? Why ?

Re: [PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Guenter Roeck]

On 05/30/2016 06:29 AM, Neil Armstrong wrote:
> Add watchdog specific driver for Amlogic Meson GXBB SoC.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>   drivers/watchdog/Makefile         |   1 +
>   drivers/watchdog/meson_gxbb_wdt.c | 277 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 278 insertions(+)
>   create mode 100644 drivers/watchdog/meson_gxbb_wdt.c
>
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 9bde095..7679d93 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o
>   obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
>   obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
>   obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
> +obj-$(CONFIG_MESON_WATCHDOG) += meson_gxbb_wdt.o

I would really prefer a separate configuration option.

>   obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
>   obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
>   obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
> diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
> new file mode 100644
> index 0000000..9619c5e
> --- /dev/null
> +++ b/drivers/watchdog/meson_gxbb_wdt.c
> @@ -0,0 +1,277 @@
> +/*
> + * This file is provided under a dual BSD/GPLv2 license.  When using or
> + * redistributing this file, you may do so under either license.
> + *
> + * GPL LICENSE SUMMARY
> + *
> + * Copyright (c) 2016 BayLibre, SAS.
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + * The full GNU General Public License is included in this distribution
> + * in the file called COPYING.
> + *
> + * BSD LICENSE
> + *
> + * Copyright (c) 2016 BayLibre, SAS.
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + *   * Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + *   * Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in
> + *     the documentation and/or other materials provided with the
> + *     distribution.
> + *   * Neither the name of Intel Corporation nor the names of its
> + *     contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/watchdog.h>

Please order include files alphabetically; that simplifies later changes
and helps finding duplicates.

> +
> +#define DEFAULT_TIMEOUT	10	/* seconds */
> +

Sure you want the default timeout to be that low ?

> +#define GXBB_WDT_CTRL_REG			0x0
> +#define GXBB_WDT_CTRL1_REG			0x4
> +#define GXBB_WDT_TCNT_REG			0x8
> +#define GXBB_WDT_RSET_REG			0xc
> +
> +#define GXBB_WDT_CTRL_EE_RESET_NOW		BIT(26)
> +
> +#define GXBB_WDT_CTRL_CLKDIV_EN			BIT(25)
> +#define GXBB_WDT_CTRL_CLK_EN			BIT(24)
> +#define GXBB_WDT_CTRL_IRQ_EN			BIT(23)
> +#define GXBB_WDT_CTRL_EE_RESET			BIT(21)
> +#define GXBB_WDT_CTRL_XTAL_SEL			(0)
> +#define GXBB_WDT_CTRL_CLK81_SEL			BIT(19)
> +#define GXBB_WDT_CTRL_EN			BIT(18)
> +#define GXBB_WDT_CTRL_DIV_MASK			(BIT(18) - 1)
> +
> +#define GXBB_WDT_CTRL1_GPIO_PULSE		BIT(17)
> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0		BIT(16)
> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1		(0)
> +#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT		(BIT(16) - 1)
> +
> +#define GXBB_WDT_TCNT_SETUP_MASK		(BIT(16) - 1)
> +#define GXBB_WDT_TCNT_CNT_SHIFT			16
> +
> +struct meson_gxbb_wdt {
> +	void __iomem *reg_base;
> +	struct watchdog_device wdt_dev;
> +	struct clk *clk;
> +};
> +
> +static int meson_gxbb_wdt_start(struct watchdog_device *wdt_dev)
> +{
> +	struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> +
> +	writel(readl(data->reg_base + GXBB_WDT_CTRL_REG) | GXBB_WDT_CTRL_EN,
> +		data->reg_base + GXBB_WDT_CTRL_REG);
> +
> +	return 0;
> +}
> +
> +static int meson_gxbb_wdt_stop(struct watchdog_device *wdt_dev)
> +{
> +	struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> +
> +	writel(readl(data->reg_base + GXBB_WDT_CTRL_REG) & ~GXBB_WDT_CTRL_EN,
> +		data->reg_base + GXBB_WDT_CTRL_REG);

Please align continuation lines with '('.

> +
> +	return 0;
> +}
> +
> +static int meson_gxbb_wdt_ping(struct watchdog_device *wdt_dev)
> +{
> +	struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> +
> +	writel(0, data->reg_base + GXBB_WDT_RSET_REG);
> +
> +	return 0;
> +}
> +
> +static int meson_gxbb_wdt_get_timeout(struct meson_gxbb_wdt *data)
> +{
> +	return (readl(data->reg_base + GXBB_WDT_TCNT_REG) &
> +			GXBB_WDT_TCNT_SETUP_MASK) / 1000;
> +}
> +
Unused function ?

> +static int meson_gxbb_wdt_set_timeout(struct watchdog_device *wdt_dev,
> +				      unsigned int timeout)
> +{
> +	struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> +
> +	meson_gxbb_wdt_ping(wdt_dev);

Is this necessary ? The infrastructure calls it after calling this function.

> +
> +	writel(timeout * 1000, data->reg_base + GXBB_WDT_TCNT_REG);

	wdt_dev->timeout = timeout;

Also, timeout may be larger than the maximum supported by the hardware,
so you need to ensure that the value your write is not larger than
GXBB_WDT_TCNT_SETUP_MASK.

> +
> +	return 0;
> +}
> +
> +static unsigned int meson_gxbb_wdt_get_timeleft(struct watchdog_device *wdt_dev)
> +{
> +	struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> +	unsigned long reg;
> +
> +	reg = readl(data->reg_base + GXBB_WDT_TCNT_REG);
> +
> +	return ((reg >> GXBB_WDT_TCNT_CNT_SHIFT)-

Space between operators, please.

> +			(reg & GXBB_WDT_TCNT_SETUP_MASK)) / 1000;
> +}
> +
> +static const struct watchdog_ops meson_gxbb_wdt_ops = {
> +	.start = meson_gxbb_wdt_start,
> +	.stop = meson_gxbb_wdt_stop,
> +	.ping = meson_gxbb_wdt_ping,
> +	.set_timeout = meson_gxbb_wdt_set_timeout,
> +	.get_timeleft = meson_gxbb_wdt_get_timeleft,
> +};
> +
> +static const struct watchdog_info meson_gxbb_wdt_info = {
> +	.identity = "meson-gxbb-wdt",

It is common here to provide a user readable string, which would be something
like "Meson GXBB Watchdog".

> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +};
> +
> +static int __maybe_unused meson_gxbb_wdt_resume(struct device *dev)
> +{
> +	struct meson_gxbb_wdt *data = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(&data->wdt_dev))
> +		meson_gxbb_wdt_start(&data->wdt_dev);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused meson_gxbb_wdt_suspend(struct device *dev)
> +{
> +	struct meson_gxbb_wdt *data = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(&data->wdt_dev))
> +		meson_gxbb_wdt_stop(&data->wdt_dev);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops meson_gxbb_wdt_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(meson_gxbb_wdt_suspend, meson_gxbb_wdt_resume)
> +};
> +
> +static const struct of_device_id meson_gxbb_wdt_dt_ids[] = {
> +	 { .compatible = "amlogic,meson-gxbb-wdt", },
> +	 { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, meson_gxbb_wdt_dt_ids);
> +
> +static int meson_gxbb_wdt_probe(struct platform_device *pdev)
> +{
> +	struct meson_gxbb_wdt *data;
> +	struct resource *res;
> +	int ret;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	data->reg_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(data->reg_base))
> +		return PTR_ERR(data->reg_base);
> +
> +	data->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(data->clk))
> +		return PTR_ERR(data->clk);
> +
> +	clk_prepare_enable(data->clk);
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	data->wdt_dev.parent = &pdev->dev;
> +	data->wdt_dev.info = &meson_gxbb_wdt_info;
> +	data->wdt_dev.ops = &meson_gxbb_wdt_ops;
> +	data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
> +	data->wdt_dev.min_timeout = 1;
> +	data->wdt_dev.timeout = DEFAULT_TIMEOUT;
> +	watchdog_set_drvdata(&data->wdt_dev, data);
> +
> +	/* Setup with 1ms timebase */
> +	writel(((clk_get_rate(data->clk) / 1000) & GXBB_WDT_CTRL_DIV_MASK) |
> +		GXBB_WDT_CTRL_EE_RESET |
> +		GXBB_WDT_CTRL_CLK_EN |
> +		GXBB_WDT_CTRL_CLKDIV_EN,
> +		data->reg_base + GXBB_WDT_CTRL_REG);
> +
> +	meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
> +
> +	ret = watchdog_register_device(&data->wdt_dev);
> +	if (ret)
> +		return ret;

clk_disable_unprepare() ?

> +
> +	return 0;
> +}
> +
> +static int meson_gxbb_wdt_remove(struct platform_device *pdev)
> +{
> +	struct meson_gxbb_wdt *data = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(&data->wdt_dev);
> +

clk_disable_unprepare() ?

> +	return 0;
> +}
> +
> +static void meson_gxbb_wdt_shutdown(struct platform_device *pdev)
> +{
> +	struct meson_gxbb_wdt *data = platform_get_drvdata(pdev);
> +
> +	meson_gxbb_wdt_stop(&data->wdt_dev);
> +}
> +
> +static struct platform_driver meson_gxbb_wdt_driver = {
> +	.probe	= meson_gxbb_wdt_probe,
> +	.remove	= meson_gxbb_wdt_remove,
> +	.shutdown = meson_gxbb_wdt_shutdown,
> +	.driver = {
> +		.name = "meson-gxbb-wdt",
> +		.pm = &meson_gxbb_wdt_pm_ops,
> +		.of_match_table	= meson_gxbb_wdt_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(meson_gxbb_wdt_driver);
> +
> +MODULE_ALIAS("platform:meson-gxbb-wdt");
> +MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
> +MODULE_DESCRIPTION("Amlogic Meson GXBB Watchdog timer driver");
> +MODULE_LICENSE("Dual BSD/GPL");
>

Re: [PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Neil Armstrong]

Hi Guenter,
On 05/30/2016 06:50 PM, Guenter Roeck wrote:
> On 05/30/2016 06:29 AM, Neil Armstrong wrote:
>> Add watchdog specific driver for Amlogic Meson GXBB SoC.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>   drivers/watchdog/Makefile         |   1 +
>>   drivers/watchdog/meson_gxbb_wdt.c | 277 ++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 278 insertions(+)
>>   create mode 100644 drivers/watchdog/meson_gxbb_wdt.c
>>
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 9bde095..7679d93 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -68,6 +68,7 @@ obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o
>>   obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
>>   obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
>>   obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
>> +obj-$(CONFIG_MESON_WATCHDOG) += meson_gxbb_wdt.o
> 
> I would really prefer a separate configuration option.

OK

> 
>>   obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
>>   obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
>>   obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
>> diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
>> new file mode 100644
>> index 0000000..9619c5e
>> --- /dev/null
>> +++ b/drivers/watchdog/meson_gxbb_wdt.c
>> @@ -0,0 +1,277 @@
>> +/*
>> + * This file is provided under a dual BSD/GPLv2 license.  When using or
>> + * redistributing this file, you may do so under either license.
>> + *
>> + * GPL LICENSE SUMMARY
>> + *
>> + * Copyright (c) 2016 BayLibre, SAS.
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of version 2 of the GNU General Public License as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
>> + * The full GNU General Public License is included in this distribution
>> + * in the file called COPYING.
>> + *
>> + * BSD LICENSE
>> + *
>> + * Copyright (c) 2016 BayLibre, SAS.
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + *   * Redistributions of source code must retain the above copyright
>> + *     notice, this list of conditions and the following disclaimer.
>> + *   * Redistributions in binary form must reproduce the above copyright
>> + *     notice, this list of conditions and the following disclaimer in
>> + *     the documentation and/or other materials provided with the
>> + *     distribution.
>> + *   * Neither the name of Intel Corporation nor the names of its
>> + *     contributors may be used to endorse or promote products derived
>> + *     from this software without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>> +#include <linux/err.h>
>> +#include <linux/module.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/clk.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +#include <linux/watchdog.h>
> 
> Please order include files alphabetically; that simplifies later changes
> and helps finding duplicates.
> 
>> +
>> +#define DEFAULT_TIMEOUT    10    /* seconds */
>> +
> 
> Sure you want the default timeout to be that low ?

I'm not sure of anything actually, what is generally set ?

> 
>> +#define GXBB_WDT_CTRL_REG            0x0
>> +#define GXBB_WDT_CTRL1_REG            0x4
>> +#define GXBB_WDT_TCNT_REG            0x8
>> +#define GXBB_WDT_RSET_REG            0xc
>> +
>> +#define GXBB_WDT_CTRL_EE_RESET_NOW        BIT(26)
>> +
>> +#define GXBB_WDT_CTRL_CLKDIV_EN            BIT(25)
>> +#define GXBB_WDT_CTRL_CLK_EN            BIT(24)
>> +#define GXBB_WDT_CTRL_IRQ_EN            BIT(23)
>> +#define GXBB_WDT_CTRL_EE_RESET            BIT(21)
>> +#define GXBB_WDT_CTRL_XTAL_SEL            (0)
>> +#define GXBB_WDT_CTRL_CLK81_SEL            BIT(19)
>> +#define GXBB_WDT_CTRL_EN            BIT(18)
>> +#define GXBB_WDT_CTRL_DIV_MASK            (BIT(18) - 1)
>> +
>> +#define GXBB_WDT_CTRL1_GPIO_PULSE        BIT(17)
>> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0        BIT(16)
>> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1        (0)
>> +#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT        (BIT(16) - 1)
>> +
>> +#define GXBB_WDT_TCNT_SETUP_MASK        (BIT(16) - 1)
>> +#define GXBB_WDT_TCNT_CNT_SHIFT            16
>> +
>> +struct meson_gxbb_wdt {
>> +    void __iomem *reg_base;
>> +    struct watchdog_device wdt_dev;
>> +    struct clk *clk;
>> +};
>> +
>> +static int meson_gxbb_wdt_start(struct watchdog_device *wdt_dev)
>> +{
>> +    struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> +    writel(readl(data->reg_base + GXBB_WDT_CTRL_REG) | GXBB_WDT_CTRL_EN,
>> +        data->reg_base + GXBB_WDT_CTRL_REG);
>> +
>> +    return 0;
>> +}
>> +
>> +static int meson_gxbb_wdt_stop(struct watchdog_device *wdt_dev)
>> +{
>> +    struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> +    writel(readl(data->reg_base + GXBB_WDT_CTRL_REG) & ~GXBB_WDT_CTRL_EN,
>> +        data->reg_base + GXBB_WDT_CTRL_REG);
> 
> Please align continuation lines with '('.
> 
>> +
>> +    return 0;
>> +}
>> +
>> +static int meson_gxbb_wdt_ping(struct watchdog_device *wdt_dev)
>> +{
>> +    struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> +    writel(0, data->reg_base + GXBB_WDT_RSET_REG);
>> +
>> +    return 0;
>> +}
>> +
>> +static int meson_gxbb_wdt_get_timeout(struct meson_gxbb_wdt *data)
>> +{
>> +    return (readl(data->reg_base + GXBB_WDT_TCNT_REG) &
>> +            GXBB_WDT_TCNT_SETUP_MASK) / 1000;
>> +}
>> +
> Unused function ?
Crap, forgot to remove this.

> 
>> +static int meson_gxbb_wdt_set_timeout(struct watchdog_device *wdt_dev,
>> +                      unsigned int timeout)
>> +{
>> +    struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> +    meson_gxbb_wdt_ping(wdt_dev);
> 
> Is this necessary ? The infrastructure calls it after calling this function.

Is it really safe to change the timeout value before calling ping ?

On the HW, there is a running counter that is reset to 0 when writing any value to the RSET register.
A comparator triggers a reset when the counter is >= the timeout value.

If somehow we change to a lower value but the counter is > the new timeout value, it will reset the system.
Calling ping right before would keep this safe.

> 
>> +
>> +    writel(timeout * 1000, data->reg_base + GXBB_WDT_TCNT_REG);
> 
>     wdt_dev->timeout = timeout;
> 
> Also, timeout may be larger than the maximum supported by the hardware,
> so you need to ensure that the value your write is not larger than
> GXBB_WDT_TCNT_SETUP_MASK.

Isn't already checked by the watchdog-core ?

> 
>> +
>> +    return 0;
>> +}
>> +
>> +static unsigned int meson_gxbb_wdt_get_timeleft(struct watchdog_device *wdt_dev)
>> +{
>> +    struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +    unsigned long reg;
>> +
>> +    reg = readl(data->reg_base + GXBB_WDT_TCNT_REG);
>> +
>> +    return ((reg >> GXBB_WDT_TCNT_CNT_SHIFT)-
> 
> Space between operators, please.
> 
>> +            (reg & GXBB_WDT_TCNT_SETUP_MASK)) / 1000;
>> +}
>> +
>> +static const struct watchdog_ops meson_gxbb_wdt_ops = {
>> +    .start = meson_gxbb_wdt_start,
>> +    .stop = meson_gxbb_wdt_stop,
>> +    .ping = meson_gxbb_wdt_ping,
>> +    .set_timeout = meson_gxbb_wdt_set_timeout,
>> +    .get_timeleft = meson_gxbb_wdt_get_timeleft,
>> +};
>> +
>> +static const struct watchdog_info meson_gxbb_wdt_info = {
>> +    .identity = "meson-gxbb-wdt",
> 
> It is common here to provide a user readable string, which would be something
> like "Meson GXBB Watchdog".
> 
Why not.

[...]

>> +
>> +static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>> +{
>> +    struct meson_gxbb_wdt *data;
>> +    struct resource *res;
>> +    int ret;
>> +
>> +    data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +    if (!data)
>> +        return -ENOMEM;
>> +
>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +    data->reg_base = devm_ioremap_resource(&pdev->dev, res);
>> +    if (IS_ERR(data->reg_base))
>> +        return PTR_ERR(data->reg_base);
>> +
>> +    data->clk = devm_clk_get(&pdev->dev, NULL);
>> +    if (IS_ERR(data->clk))
>> +        return PTR_ERR(data->clk);
>> +
>> +    clk_prepare_enable(data->clk);
>> +
>> +    platform_set_drvdata(pdev, data);
>> +
>> +    data->wdt_dev.parent = &pdev->dev;
>> +    data->wdt_dev.info = &meson_gxbb_wdt_info;
>> +    data->wdt_dev.ops = &meson_gxbb_wdt_ops;
>> +    data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
>> +    data->wdt_dev.min_timeout = 1;
>> +    data->wdt_dev.timeout = DEFAULT_TIMEOUT;
>> +    watchdog_set_drvdata(&data->wdt_dev, data);
>> +
>> +    /* Setup with 1ms timebase */
>> +    writel(((clk_get_rate(data->clk) / 1000) & GXBB_WDT_CTRL_DIV_MASK) |
>> +        GXBB_WDT_CTRL_EE_RESET |
>> +        GXBB_WDT_CTRL_CLK_EN |
>> +        GXBB_WDT_CTRL_CLKDIV_EN,
>> +        data->reg_base + GXBB_WDT_CTRL_REG);
>> +
>> +    meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
>> +
>> +    ret = watchdog_register_device(&data->wdt_dev);
>> +    if (ret)
>> +        return ret;
> 
> clk_disable_unprepare() ?

Sure.

> 
>> +
>> +    return 0;
>> +}
>> +
>> +static int meson_gxbb_wdt_remove(struct platform_device *pdev)
>> +{
>> +    struct meson_gxbb_wdt *data = platform_get_drvdata(pdev);
>> +
>> +    watchdog_unregister_device(&data->wdt_dev);
>> +
> 
> clk_disable_unprepare() ?
Same.

Thanks,
Neil

Re: [PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Guenter Roeck]

On Tue, May 31, 2016 at 11:57:03AM +0200, Neil Armstrong wrote:
> 
> Hi Guenter,
> On 05/30/2016 06:50 PM, Guenter Roeck wrote:
> > On 05/30/2016 06:29 AM, Neil Armstrong wrote:
> >> Add watchdog specific driver for Amlogic Meson GXBB SoC.
> >>
> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> >> ---
> >>   drivers/watchdog/Makefile         |   1 +
> >>   drivers/watchdog/meson_gxbb_wdt.c | 277 ++++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 278 insertions(+)
> >>   create mode 100644 drivers/watchdog/meson_gxbb_wdt.c
> >>
> >> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> >> index 9bde095..7679d93 100644
> >> --- a/drivers/watchdog/Makefile
> >> +++ b/drivers/watchdog/Makefile
> >> @@ -68,6 +68,7 @@ obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o
> >>   obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
> >>   obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
> >>   obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
> >> +obj-$(CONFIG_MESON_WATCHDOG) += meson_gxbb_wdt.o
> > 
> > I would really prefer a separate configuration option.
> 
> OK
> 
> > 
> >>   obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
> >>   obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
> >>   obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
> >> diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
> >> new file mode 100644
> >> index 0000000..9619c5e
> >> --- /dev/null
> >> +++ b/drivers/watchdog/meson_gxbb_wdt.c
> >> @@ -0,0 +1,277 @@
> >> +/*
> >> + * This file is provided under a dual BSD/GPLv2 license.  When using or
> >> + * redistributing this file, you may do so under either license.
> >> + *
> >> + * GPL LICENSE SUMMARY
> >> + *
> >> + * Copyright (c) 2016 BayLibre, SAS.
> >> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of version 2 of the GNU General Public License as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope that it will be useful, but
> >> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> + * General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> >> + * The full GNU General Public License is included in this distribution
> >> + * in the file called COPYING.
> >> + *
> >> + * BSD LICENSE
> >> + *
> >> + * Copyright (c) 2016 BayLibre, SAS.
> >> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> >> + *
> >> + * Redistribution and use in source and binary forms, with or without
> >> + * modification, are permitted provided that the following conditions
> >> + * are met:
> >> + *
> >> + *   * Redistributions of source code must retain the above copyright
> >> + *     notice, this list of conditions and the following disclaimer.
> >> + *   * Redistributions in binary form must reproduce the above copyright
> >> + *     notice, this list of conditions and the following disclaimer in
> >> + *     the documentation and/or other materials provided with the
> >> + *     distribution.
> >> + *   * Neither the name of Intel Corporation nor the names of its
> >> + *     contributors may be used to endorse or promote products derived
> >> + *     from this software without specific prior written permission.
> >> + *
> >> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> >> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> >> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> >> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> >> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> >> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> >> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> >> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> >> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> >> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> >> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >> + */
> >> +#include <linux/err.h>
> >> +#include <linux/module.h>
> >> +#include <linux/io.h>
> >> +#include <linux/of.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/types.h>
> >> +#include <linux/watchdog.h>
> > 
> > Please order include files alphabetically; that simplifies later changes
> > and helps finding duplicates.
> > 
> >> +
> >> +#define DEFAULT_TIMEOUT    10    /* seconds */
> >> +
> > 
> > Sure you want the default timeout to be that low ?
> 
> I'm not sure of anything actually, what is generally set ?
> 

30 seconds is quite common. I don't really mind 10 seconds, if that makes
sense for this platform, but in some cases it is a bit low.


> > 
> >> +#define GXBB_WDT_CTRL_REG            0x0
> >> +#define GXBB_WDT_CTRL1_REG            0x4
> >> +#define GXBB_WDT_TCNT_REG            0x8
> >> +#define GXBB_WDT_RSET_REG            0xc
> >> +
> >> +#define GXBB_WDT_CTRL_EE_RESET_NOW        BIT(26)
> >> +
> >> +#define GXBB_WDT_CTRL_CLKDIV_EN            BIT(25)
> >> +#define GXBB_WDT_CTRL_CLK_EN            BIT(24)
> >> +#define GXBB_WDT_CTRL_IRQ_EN            BIT(23)
> >> +#define GXBB_WDT_CTRL_EE_RESET            BIT(21)
> >> +#define GXBB_WDT_CTRL_XTAL_SEL            (0)
> >> +#define GXBB_WDT_CTRL_CLK81_SEL            BIT(19)
> >> +#define GXBB_WDT_CTRL_EN            BIT(18)
> >> +#define GXBB_WDT_CTRL_DIV_MASK            (BIT(18) - 1)
> >> +
> >> +#define GXBB_WDT_CTRL1_GPIO_PULSE        BIT(17)
> >> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0        BIT(16)
> >> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1        (0)
> >> +#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT        (BIT(16) - 1)
> >> +
> >> +#define GXBB_WDT_TCNT_SETUP_MASK        (BIT(16) - 1)
> >> +#define GXBB_WDT_TCNT_CNT_SHIFT            16
> >> +
> >> +struct meson_gxbb_wdt {
> >> +    void __iomem *reg_base;
> >> +    struct watchdog_device wdt_dev;
> >> +    struct clk *clk;
> >> +};
> >> +
> >> +static int meson_gxbb_wdt_start(struct watchdog_device *wdt_dev)
> >> +{
> >> +    struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> >> +
> >> +    writel(readl(data->reg_base + GXBB_WDT_CTRL_REG) | GXBB_WDT_CTRL_EN,
> >> +        data->reg_base + GXBB_WDT_CTRL_REG);
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int meson_gxbb_wdt_stop(struct watchdog_device *wdt_dev)
> >> +{
> >> +    struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> >> +
> >> +    writel(readl(data->reg_base + GXBB_WDT_CTRL_REG) & ~GXBB_WDT_CTRL_EN,
> >> +        data->reg_base + GXBB_WDT_CTRL_REG);
> > 
> > Please align continuation lines with '('.
> > 
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int meson_gxbb_wdt_ping(struct watchdog_device *wdt_dev)
> >> +{
> >> +    struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> >> +
> >> +    writel(0, data->reg_base + GXBB_WDT_RSET_REG);
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int meson_gxbb_wdt_get_timeout(struct meson_gxbb_wdt *data)
> >> +{
> >> +    return (readl(data->reg_base + GXBB_WDT_TCNT_REG) &
> >> +            GXBB_WDT_TCNT_SETUP_MASK) / 1000;
> >> +}
> >> +
> > Unused function ?
> Crap, forgot to remove this.
> 
> > 
> >> +static int meson_gxbb_wdt_set_timeout(struct watchdog_device *wdt_dev,
> >> +                      unsigned int timeout)
> >> +{
> >> +    struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> >> +
> >> +    meson_gxbb_wdt_ping(wdt_dev);
> > 
> > Is this necessary ? The infrastructure calls it after calling this function.
> 
> Is it really safe to change the timeout value before calling ping ?
> 
> On the HW, there is a running counter that is reset to 0 when writing any value to the RSET register.
> A comparator triggers a reset when the counter is >= the timeout value.
> 
> If somehow we change to a lower value but the counter is > the new timeout value, it will reset the system.
> Calling ping right before would keep this safe.
> 
Ok.

> > 
> >> +
> >> +    writel(timeout * 1000, data->reg_base + GXBB_WDT_TCNT_REG);
> > 
> >     wdt_dev->timeout = timeout;
> > 
> > Also, timeout may be larger than the maximum supported by the hardware,
> > so you need to ensure that the value your write is not larger than
> > GXBB_WDT_TCNT_SETUP_MASK.
> 
> Isn't already checked by the watchdog-core ?
> 

Not anymore if the maximum hardware timeout is specified.

The difficulty in this case is that 'timeout' no longer reflects
the hardware timeout, but the logical timeout set from user space.
Maybe we can come up with a better mechanism (maybe two separate variables),
but for now drivers which set the maximum HW timeout have to check the
value range.

Thanks,
Guenter

Re: [PATCH 2/3] dt-bindings: watchdog: Add Meson GXBB Watchdog bindings

[Rob Herring]

On Mon, May 30, 2016 at 04:12:06PM +0200, Neil Armstrong wrote:
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  .../devicetree/bindings/watchdog/meson-gxbb-wdt.txt      | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt

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