i.MX8QXP is an ARMv8 Cortex-A35 SoC with a Cortex-M4 system controller inside,
the system controller manages overall power, clock, secure RTC and watchdog
etc., so Linux kernel running on A35 needs to communicate with system
controller for watchdog operation, this system controller watchdog will
call SMC to trap to EL3 secure world ARM-Trusted-Firmware and then it will
request system controller to do the watchdog operation via IPC.
This V2 patch series addresses all the comments from V1, and change log is inside
each patch of this V2 series.
Below are the patch dependency:
The dt-binding and imx8qxp.dtsi patch are based on my thermal patch series as below:
https://patchwork.kernel.org/patch/10800699/
Anson Huang (4):
dt-bindings: fsl: scu: add watchdog binding
watchdog: imx_sc: Add i.MX system controller watchdog support
arm64: defconfig: add support for i.MX system controller watchdog
arm64: dts: freescale: imx8qxp: add watchdog support
.../devicetree/bindings/arm/freescale/fsl,scu.txt | 10 +
arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 4 +
arch/arm64/configs/defconfig | 1 +
drivers/watchdog/Kconfig | 13 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/imx_sc_wdt.c | 210 +++++++++++++++++++++
6 files changed, 239 insertions(+)
create mode 100644 drivers/watchdog/imx_sc_wdt.c
--
2.7.4
Enable CONFIG_IMX_SC_WDT as module to support i.MX system
controller watchdog.
Signed-off-by: Anson Huang <[email protected]>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 2d9c390..690f4ba 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -427,6 +427,7 @@ CONFIG_WATCHDOG=y
CONFIG_ARM_SP805_WATCHDOG=y
CONFIG_S3C2410_WATCHDOG=y
CONFIG_IMX2_WDT=y
+CONFIG_IMX_SC_WDT=m
CONFIG_MESON_GXBB_WATCHDOG=m
CONFIG_MESON_WATCHDOG=m
CONFIG_RENESAS_WDT=y
--
2.7.4
Add i.MX8QXP system controller watchdog binding.
Signed-off-by: Anson Huang <[email protected]>
---
Changes since V1:
- update dts node name to "watchdog";
---
Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
index 4b79751..f388ec6 100644
--- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -136,6 +136,12 @@ Required properties:
resource id for thermal driver to get temperature via
SCU IPC.
+Watchdog bindings based on SCU Message Protocol
+------------------------------------------------------------
+
+Required properties:
+- compatible: should be "fsl,imx8qxp-sc-wdt";
+
Example (imx8qxp):
-------------
lsio_mu1: mailbox@5d1c0000 {
@@ -188,6 +194,10 @@ firmware {
tsens-num = <1>;
#thermal-sensor-cells = <1>;
};
+
+ watchdog: watchdog {
+ compatible = "fsl,imx8qxp-sc-wdt";
+ };
};
};
--
2.7.4
i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
inside, the system controller is in charge of controlling power,
clock and watchdog etc..
This patch adds i.MX system controller watchdog driver support,
watchdog operation needs to be done in secure EL3 mode via
ARM-Trusted-Firmware, using SMC call, CPU will trap into
ARM-Trusted-Firmware and then it will request system controller
to do watchdog operation via IPC.
Signed-off-by: Anson Huang <[email protected]>
---
Changes since V1:
- use watchdog_active() instead of comparing watchdog status directly;
- use devm_kzalloc instead of using static variable;
- remove unnecessary wdog set driver data in .probe;
- use watchdog_stop_on_unregister() instead of .remove callback;
- use watchdog_stop_on_reboot() instead of .shutdown callback;
- use SIMPLE_DEV_PM_OPS() to simply the PM code.
---
drivers/watchdog/Kconfig | 13 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/imx_sc_wdt.c | 185 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 199 insertions(+)
create mode 100644 drivers/watchdog/imx_sc_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 65c3c42..5c5b8ba 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -625,6 +625,19 @@ config IMX2_WDT
To compile this driver as a module, choose M here: the
module will be called imx2_wdt.
+config IMX_SC_WDT
+ tristate "IMX SC Watchdog"
+ depends on ARCH_MXC || COMPILE_TEST
+ select WATCHDOG_CORE
+ help
+ This is the driver for the system controller watchdog
+ on the NXP i.MX SoCs with system controller inside.
+ If you have one of these processors and wish to have
+ watchdog support enabled, say Y, otherwise say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called imx_sc_wdt.
+
config UX500_WATCHDOG
tristate "ST-Ericsson Ux500 watchdog"
depends on MFD_DB8500_PRCMU
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 4e78a8c..0c9da63 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
+obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c
new file mode 100644
index 0000000..a234a0c
--- /dev/null
+++ b/drivers/watchdog/imx_sc_wdt.c
@@ -0,0 +1,185 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018-2019 NXP.
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/io.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/watchdog.h>
+
+#define DEFAULT_TIMEOUT 60
+/*
+ * Software timer tick implemented in scfw side, support 10ms to 0xffffffff ms
+ * in theory, but for normal case, 1s~128s is enough, you can change this max
+ * value in case it's not enough.
+ */
+#define MAX_TIMEOUT 128
+
+#define IMX_SIP_TIMER 0xC2000002
+#define IMX_SIP_TIMER_START_WDOG 0x01
+#define IMX_SIP_TIMER_STOP_WDOG 0x02
+#define IMX_SIP_TIMER_SET_WDOG_ACT 0x03
+#define IMX_SIP_TIMER_PING_WDOG 0x04
+#define IMX_SIP_TIMER_SET_TIMEOUT_WDOG 0x05
+#define IMX_SIP_TIMER_GET_WDOG_STAT 0x06
+#define IMX_SIP_TIMER_SET_PRETIME_WDOG 0x07
+
+#define SC_TIMER_WDOG_ACTION_PARTITION 0
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0000);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static unsigned int timeout = DEFAULT_TIMEOUT;
+module_param(timeout, uint, 0000);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
+ __MODULE_STRING(DEFAULT_TIMEOUT) ")");
+
+static int imx_sc_wdt_ping(struct watchdog_device *wdog)
+{
+ struct arm_smccc_res res;
+
+ arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_PING_WDOG,
+ 0, 0, 0, 0, 0, 0, &res);
+
+ return res.a0;
+}
+
+static int imx_sc_wdt_start(struct watchdog_device *wdog)
+{
+ struct arm_smccc_res res;
+
+ arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_START_WDOG,
+ 0, 0, 0, 0, 0, 0, &res);
+ if (res.a0)
+ return res.a0;
+
+ arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_SET_WDOG_ACT,
+ SC_TIMER_WDOG_ACTION_PARTITION,
+ 0, 0, 0, 0, 0, &res);
+ return res.a0;
+}
+
+static int imx_sc_wdt_stop(struct watchdog_device *wdog)
+{
+ struct arm_smccc_res res;
+
+ arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_STOP_WDOG,
+ 0, 0, 0, 0, 0, 0, &res);
+
+ return res.a0;
+}
+
+static int imx_sc_wdt_set_timeout(struct watchdog_device *wdog,
+ unsigned int timeout)
+{
+ struct arm_smccc_res res;
+
+ wdog->timeout = timeout;
+ arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_SET_TIMEOUT_WDOG,
+ timeout * 1000, 0, 0, 0, 0, 0, &res);
+
+ return res.a0;
+}
+
+static const struct watchdog_ops imx_sc_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = imx_sc_wdt_start,
+ .stop = imx_sc_wdt_stop,
+ .ping = imx_sc_wdt_ping,
+ .set_timeout = imx_sc_wdt_set_timeout,
+};
+
+static const struct watchdog_info imx_sc_wdt_info = {
+ .identity = "i.MX SC watchdog timer",
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
+ WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT,
+};
+
+static int imx_sc_wdt_probe(struct platform_device *pdev)
+{
+ struct watchdog_device *imx_sc_wdd;
+ int ret;
+
+ imx_sc_wdd = devm_kzalloc(&pdev->dev, sizeof(*imx_sc_wdd), GFP_KERNEL);
+ if (!imx_sc_wdd)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, imx_sc_wdd);
+
+ imx_sc_wdd->info = &imx_sc_wdt_info;
+ imx_sc_wdd->ops = &imx_sc_wdt_ops;
+ imx_sc_wdd->min_timeout = 1;
+ imx_sc_wdd->max_timeout = MAX_TIMEOUT;
+ imx_sc_wdd->parent = &pdev->dev;
+
+ ret = watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT, &pdev->dev);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to init the wdog timeout:%d\n",
+ ret);
+ return ret;
+ }
+
+ watchdog_stop_on_reboot(imx_sc_wdd);
+ watchdog_stop_on_unregister(imx_sc_wdd);
+
+ ret = devm_watchdog_register_device(&pdev->dev, imx_sc_wdd);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to register watchdog device\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct of_device_id imx_sc_wdt_dt_ids[] = {
+ { .compatible = "fsl,imx8qxp-sc-wdt", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_sc_wdt_dt_ids);
+
+static int __maybe_unused imx_sc_wdt_suspend(struct device *dev)
+{
+ struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
+
+ if (watchdog_active(imx_sc_wdd))
+ imx_sc_wdt_stop(imx_sc_wdd);
+
+ return 0;
+}
+
+static int __maybe_unused imx_sc_wdt_resume(struct device *dev)
+{
+ struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
+
+ if (watchdog_active(imx_sc_wdd))
+ imx_sc_wdt_start(imx_sc_wdd);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(imx_sc_wdt_pm_ops,
+ imx_sc_wdt_suspend, imx_sc_wdt_resume);
+
+static struct platform_driver imx_sc_wdt_driver = {
+ .probe = imx_sc_wdt_probe,
+ .driver = {
+ .name = "imx-sc-wdt",
+ .of_match_table = imx_sc_wdt_dt_ids,
+ .pm = &imx_sc_wdt_pm_ops,
+ },
+};
+
+module_platform_driver(imx_sc_wdt_driver);
+
+MODULE_AUTHOR("Robin Gong <[email protected]>");
+MODULE_DESCRIPTION("NXP i.MX system controller watchdog driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
Add i.MX8QXP system controller watchdog support.
Signed-off-by: Anson Huang <[email protected]>
---
Changes since V1:
- use watchdog instead of wdog.
---
arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
index 88eb04b..bfb77a5 100644
--- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
@@ -123,6 +123,10 @@
tsens-num = <1>;
#thermal-sensor-cells = <1>;
};
+
+ watchdog: watchdog {
+ compatible = "fsl,imx8qxp-sc-wdt";
+ };
};
timer {
--
2.7.4
On 2/17/19 10:53 PM, Anson Huang wrote:
> i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
> inside, the system controller is in charge of controlling power,
> clock and watchdog etc..
>
> This patch adds i.MX system controller watchdog driver support,
> watchdog operation needs to be done in secure EL3 mode via
> ARM-Trusted-Firmware, using SMC call, CPU will trap into
> ARM-Trusted-Firmware and then it will request system controller
> to do watchdog operation via IPC.
>
> Signed-off-by: Anson Huang <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> Changes since V1:
> - use watchdog_active() instead of comparing watchdog status directly;
> - use devm_kzalloc instead of using static variable;
> - remove unnecessary wdog set driver data in .probe;
> - use watchdog_stop_on_unregister() instead of .remove callback;
> - use watchdog_stop_on_reboot() instead of .shutdown callback;
> - use SIMPLE_DEV_PM_OPS() to simply the PM code.
> ---
> drivers/watchdog/Kconfig | 13 +++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/imx_sc_wdt.c | 185 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 199 insertions(+)
> create mode 100644 drivers/watchdog/imx_sc_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 65c3c42..5c5b8ba 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -625,6 +625,19 @@ config IMX2_WDT
> To compile this driver as a module, choose M here: the
> module will be called imx2_wdt.
>
> +config IMX_SC_WDT
> + tristate "IMX SC Watchdog"
> + depends on ARCH_MXC || COMPILE_TEST
> + select WATCHDOG_CORE
> + help
> + This is the driver for the system controller watchdog
> + on the NXP i.MX SoCs with system controller inside.
> + If you have one of these processors and wish to have
> + watchdog support enabled, say Y, otherwise say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called imx_sc_wdt.
> +
> config UX500_WATCHDOG
> tristate "ST-Ericsson Ux500 watchdog"
> depends on MFD_DB8500_PRCMU
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 4e78a8c..0c9da63 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
> obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
> obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
> obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
> +obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
> obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
> obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
> obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
> diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c
> new file mode 100644
> index 0000000..a234a0c
> --- /dev/null
> +++ b/drivers/watchdog/imx_sc_wdt.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018-2019 NXP.
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/io.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/watchdog.h>
> +
> +#define DEFAULT_TIMEOUT 60
> +/*
> + * Software timer tick implemented in scfw side, support 10ms to 0xffffffff ms
> + * in theory, but for normal case, 1s~128s is enough, you can change this max
> + * value in case it's not enough.
> + */
> +#define MAX_TIMEOUT 128
> +
> +#define IMX_SIP_TIMER 0xC2000002
> +#define IMX_SIP_TIMER_START_WDOG 0x01
> +#define IMX_SIP_TIMER_STOP_WDOG 0x02
> +#define IMX_SIP_TIMER_SET_WDOG_ACT 0x03
> +#define IMX_SIP_TIMER_PING_WDOG 0x04
> +#define IMX_SIP_TIMER_SET_TIMEOUT_WDOG 0x05
> +#define IMX_SIP_TIMER_GET_WDOG_STAT 0x06
> +#define IMX_SIP_TIMER_SET_PRETIME_WDOG 0x07
> +
> +#define SC_TIMER_WDOG_ACTION_PARTITION 0
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0000);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static unsigned int timeout = DEFAULT_TIMEOUT;
> +module_param(timeout, uint, 0000);
> +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
> + __MODULE_STRING(DEFAULT_TIMEOUT) ")");
> +
> +static int imx_sc_wdt_ping(struct watchdog_device *wdog)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_PING_WDOG,
> + 0, 0, 0, 0, 0, 0, &res);
> +
> + return res.a0;
> +}
> +
> +static int imx_sc_wdt_start(struct watchdog_device *wdog)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_START_WDOG,
> + 0, 0, 0, 0, 0, 0, &res);
> + if (res.a0)
> + return res.a0;
> +
> + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_SET_WDOG_ACT,
> + SC_TIMER_WDOG_ACTION_PARTITION,
> + 0, 0, 0, 0, 0, &res);
> + return res.a0;
> +}
> +
> +static int imx_sc_wdt_stop(struct watchdog_device *wdog)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_STOP_WDOG,
> + 0, 0, 0, 0, 0, 0, &res);
> +
> + return res.a0;
> +}
> +
> +static int imx_sc_wdt_set_timeout(struct watchdog_device *wdog,
> + unsigned int timeout)
> +{
> + struct arm_smccc_res res;
> +
> + wdog->timeout = timeout;
> + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_SET_TIMEOUT_WDOG,
> + timeout * 1000, 0, 0, 0, 0, 0, &res);
> +
> + return res.a0;
> +}
> +
> +static const struct watchdog_ops imx_sc_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = imx_sc_wdt_start,
> + .stop = imx_sc_wdt_stop,
> + .ping = imx_sc_wdt_ping,
> + .set_timeout = imx_sc_wdt_set_timeout,
> +};
> +
> +static const struct watchdog_info imx_sc_wdt_info = {
> + .identity = "i.MX SC watchdog timer",
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> + WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT,
> +};
> +
> +static int imx_sc_wdt_probe(struct platform_device *pdev)
> +{
> + struct watchdog_device *imx_sc_wdd;
> + int ret;
> +
> + imx_sc_wdd = devm_kzalloc(&pdev->dev, sizeof(*imx_sc_wdd), GFP_KERNEL);
> + if (!imx_sc_wdd)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, imx_sc_wdd);
> +
> + imx_sc_wdd->info = &imx_sc_wdt_info;
> + imx_sc_wdd->ops = &imx_sc_wdt_ops;
> + imx_sc_wdd->min_timeout = 1;
> + imx_sc_wdd->max_timeout = MAX_TIMEOUT;
> + imx_sc_wdd->parent = &pdev->dev;
> +
> + ret = watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT, &pdev->dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to init the wdog timeout:%d\n",
> + ret);
> + return ret;
> + }
> +
> + watchdog_stop_on_reboot(imx_sc_wdd);
> + watchdog_stop_on_unregister(imx_sc_wdd);
> +
> + ret = devm_watchdog_register_device(&pdev->dev, imx_sc_wdd);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register watchdog device\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id imx_sc_wdt_dt_ids[] = {
> + { .compatible = "fsl,imx8qxp-sc-wdt", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_sc_wdt_dt_ids);
> +
> +static int __maybe_unused imx_sc_wdt_suspend(struct device *dev)
> +{
> + struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
> +
> + if (watchdog_active(imx_sc_wdd))
> + imx_sc_wdt_stop(imx_sc_wdd);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused imx_sc_wdt_resume(struct device *dev)
> +{
> + struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
> +
> + if (watchdog_active(imx_sc_wdd))
> + imx_sc_wdt_start(imx_sc_wdd);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(imx_sc_wdt_pm_ops,
> + imx_sc_wdt_suspend, imx_sc_wdt_resume);
> +
> +static struct platform_driver imx_sc_wdt_driver = {
> + .probe = imx_sc_wdt_probe,
> + .driver = {
> + .name = "imx-sc-wdt",
> + .of_match_table = imx_sc_wdt_dt_ids,
> + .pm = &imx_sc_wdt_pm_ops,
> + },
> +};
> +
> +module_platform_driver(imx_sc_wdt_driver);
> +
> +MODULE_AUTHOR("Robin Gong <[email protected]>");
> +MODULE_DESCRIPTION("NXP i.MX system controller watchdog driver");
> +MODULE_LICENSE("GPL v2");
>
On Mon, Feb 18, 2019 at 06:53:48AM +0000, Anson Huang wrote:
> Add i.MX8QXP system controller watchdog binding.
>
> Signed-off-by: Anson Huang <[email protected]>
> ---
> Changes since V1:
> - update dts node name to "watchdog";
> ---
> Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> index 4b79751..f388ec6 100644
> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> @@ -136,6 +136,12 @@ Required properties:
> resource id for thermal driver to get temperature via
> SCU IPC.
>
> +Watchdog bindings based on SCU Message Protocol
> +------------------------------------------------------------
> +
> +Required properties:
> +- compatible: should be "fsl,imx8qxp-sc-wdt";
> +
> Example (imx8qxp):
> -------------
> lsio_mu1: mailbox@5d1c0000 {
> @@ -188,6 +194,10 @@ firmware {
> tsens-num = <1>;
> #thermal-sensor-cells = <1>;
> };
> +
> + watchdog: watchdog {
> + compatible = "fsl,imx8qxp-sc-wdt";
As-is, there's no reason for this to be in DT. The parent node's driver
can instantiate the wdog.
> + };
> };
> };
>
> --
> 2.7.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 2/22/19 11:52 AM, Rob Herring wrote:
> On Mon, Feb 18, 2019 at 06:53:48AM +0000, Anson Huang wrote:
>> Add i.MX8QXP system controller watchdog binding.
>>
>> Signed-off-by: Anson Huang <[email protected]>
>> ---
>> Changes since V1:
>> - update dts node name to "watchdog";
>> ---
>> Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
>> index 4b79751..f388ec6 100644
>> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
>> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
>> @@ -136,6 +136,12 @@ Required properties:
>> resource id for thermal driver to get temperature via
>> SCU IPC.
>>
>> +Watchdog bindings based on SCU Message Protocol
>> +------------------------------------------------------------
>> +
>> +Required properties:
>> +- compatible: should be "fsl,imx8qxp-sc-wdt";
>> +
>> Example (imx8qxp):
>> -------------
>> lsio_mu1: mailbox@5d1c0000 {
>> @@ -188,6 +194,10 @@ firmware {
>> tsens-num = <1>;
>> #thermal-sensor-cells = <1>;
>> };
>> +
>> + watchdog: watchdog {
>> + compatible = "fsl,imx8qxp-sc-wdt";
>
> As-is, there's no reason for this to be in DT. The parent node's driver
> can instantiate the wdog.
>
As the driver is currently written, you are correct, since it doesn't accept
watchdog timeout configuration through devicetree.
Question is if that is intended. Is it ?
Thanks,
Guenter
Hi, Guenter/Rob
Best Regards!
Anson Huang
> -----Original Message-----
> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
> Roeck
> Sent: 2019年2月24日 1:08
> To: Rob Herring <[email protected]>; Anson Huang <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Aisheng Dong <[email protected]>; [email protected]; Daniel
> Baluta <[email protected]>; Andy Gross <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>
> Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog
> binding
>
> On 2/22/19 11:52 AM, Rob Herring wrote:
> > On Mon, Feb 18, 2019 at 06:53:48AM +0000, Anson Huang wrote:
> >> Add i.MX8QXP system controller watchdog binding.
> >>
> >> Signed-off-by: Anson Huang <[email protected]>
> >> ---
> >> Changes since V1:
> >> - update dts node name to "watchdog";
> >> ---
> >> Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 10
> ++++++++++
> >> 1 file changed, 10 insertions(+)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >> b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >> index 4b79751..f388ec6 100644
> >> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >> @@ -136,6 +136,12 @@ Required properties:
> >> resource id for thermal driver to get temperature
> via
> >> SCU IPC.
> >>
> >> +Watchdog bindings based on SCU Message Protocol
> >> +------------------------------------------------------------
> >> +
> >> +Required properties:
> >> +- compatible: should be "fsl,imx8qxp-sc-wdt";
> >> +
> >> Example (imx8qxp):
> >> -------------
> >> lsio_mu1: mailbox@5d1c0000 {
> >> @@ -188,6 +194,10 @@ firmware {
> >> tsens-num = <1>;
> >> #thermal-sensor-cells = <1>;
> >> };
> >> +
> >> + watchdog: watchdog {
> >> + compatible = "fsl,imx8qxp-sc-wdt";
> >
> > As-is, there's no reason for this to be in DT. The parent node's
> > driver can instantiate the wdog.
> >
>
> As the driver is currently written, you are correct, since it doesn't accept
> watchdog timeout configuration through devicetree.
>
> Question is if that is intended. Is it ?
I am a little confused, do you mean we no need to have "watchdog" node in side "scu"
node? Or we need to modify the watchdog node's compatible string to " fsl,imx-sc-wdt" to make
it more generic for other platforms? If yes, I can resend the patch series to modify it.
Anson.
>
> Thanks,
> Guenter
On 2/23/19 7:04 PM, Anson Huang wrote:
> Hi, Guenter/Rob
>
> Best Regards!
> Anson Huang
>
>> -----Original Message-----
>> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
>> Roeck
>> Sent: 2019年2月24日 1:08
>> To: Rob Herring <[email protected]>; Anson Huang <[email protected]>
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> Aisheng Dong <[email protected]>; [email protected]; Daniel
>> Baluta <[email protected]>; Andy Gross <[email protected]>;
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; linux-arm-
>> [email protected]; [email protected]; dl-linux-imx
>> <[email protected]>
>> Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog
>> binding
>>
>> On 2/22/19 11:52 AM, Rob Herring wrote:
>>> On Mon, Feb 18, 2019 at 06:53:48AM +0000, Anson Huang wrote:
>>>> Add i.MX8QXP system controller watchdog binding.
>>>>
>>>> Signed-off-by: Anson Huang <[email protected]>
>>>> ---
>>>> Changes since V1:
>>>> - update dts node name to "watchdog";
>>>> ---
>>>> Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 10
>> ++++++++++
>>>> 1 file changed, 10 insertions(+)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
>>>> b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
>>>> index 4b79751..f388ec6 100644
>>>> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
>>>> @@ -136,6 +136,12 @@ Required properties:
>>>> resource id for thermal driver to get temperature
>> via
>>>> SCU IPC.
>>>>
>>>> +Watchdog bindings based on SCU Message Protocol
>>>> +------------------------------------------------------------
>>>> +
>>>> +Required properties:
>>>> +- compatible: should be "fsl,imx8qxp-sc-wdt";
>>>> +
>>>> Example (imx8qxp):
>>>> -------------
>>>> lsio_mu1: mailbox@5d1c0000 {
>>>> @@ -188,6 +194,10 @@ firmware {
>>>> tsens-num = <1>;
>>>> #thermal-sensor-cells = <1>;
>>>> };
>>>> +
>>>> + watchdog: watchdog {
>>>> + compatible = "fsl,imx8qxp-sc-wdt";
>>>
>>> As-is, there's no reason for this to be in DT. The parent node's
>>> driver can instantiate the wdog.
>>>
>>
>> As the driver is currently written, you are correct, since it doesn't accept
>> watchdog timeout configuration through devicetree.
>>
>> Question is if that is intended. Is it ?
>
> I am a little confused, do you mean we no need to have "watchdog" node in side "scu"
> node? Or we need to modify the watchdog node's compatible string to " fsl,imx-sc-wdt" to make
> it more generic for other platforms? If yes, I can resend the patch series to modify it.
>
I think Rob suggested that the SCU parent driver should instantiate the watchdog
without explicit watchdog node. That would be possible, but it currently uses
devm_of_platform_populate() to do the instantiation, and changing that would
be a mess. Besides, it does sem to me that your suggested node would describe
the hardware, so I am not sure I understand the reasoning.
For my part I referred to
watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT, &pdev->dev);
in the driver, which guarantees that the timeout property will not be used
to set the timeout. A more common implementation would have been
imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
ret = watchdog_init_timeout(imx_sc_wdd, timeout, &pdev->dev);
where 'timeout' is the module parameter. Which is actually not used in your driver.
Hmm ... I wasn't careful enough with my review. The timeout initialization as-is
doesn't make sense. I'll comment on that in the patch.
Guenter
> Anson.
>
>>
>> Thanks,
>> Guenter
On 2/17/19 10:53 PM, Anson Huang wrote:
> i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
> inside, the system controller is in charge of controlling power,
> clock and watchdog etc..
>
> This patch adds i.MX system controller watchdog driver support,
> watchdog operation needs to be done in secure EL3 mode via
> ARM-Trusted-Firmware, using SMC call, CPU will trap into
> ARM-Trusted-Firmware and then it will request system controller
> to do watchdog operation via IPC.
>
> Signed-off-by: Anson Huang <[email protected]>
Sorry, but I have to take back my Reviewed-by: Comments below.
Sorry, I completely missed that earlier.
> ---
> Changes since V1:
> - use watchdog_active() instead of comparing watchdog status directly;
> - use devm_kzalloc instead of using static variable;
> - remove unnecessary wdog set driver data in .probe;
> - use watchdog_stop_on_unregister() instead of .remove callback;
> - use watchdog_stop_on_reboot() instead of .shutdown callback;
> - use SIMPLE_DEV_PM_OPS() to simply the PM code.
> ---
> drivers/watchdog/Kconfig | 13 +++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/imx_sc_wdt.c | 185 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 199 insertions(+)
> create mode 100644 drivers/watchdog/imx_sc_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 65c3c42..5c5b8ba 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -625,6 +625,19 @@ config IMX2_WDT
> To compile this driver as a module, choose M here: the
> module will be called imx2_wdt.
>
> +config IMX_SC_WDT
> + tristate "IMX SC Watchdog"
> + depends on ARCH_MXC || COMPILE_TEST
> + select WATCHDOG_CORE
> + help
> + This is the driver for the system controller watchdog
> + on the NXP i.MX SoCs with system controller inside.
> + If you have one of these processors and wish to have
> + watchdog support enabled, say Y, otherwise say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called imx_sc_wdt.
> +
> config UX500_WATCHDOG
> tristate "ST-Ericsson Ux500 watchdog"
> depends on MFD_DB8500_PRCMU
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 4e78a8c..0c9da63 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
> obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
> obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
> obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
> +obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
> obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
> obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
> obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
> diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c
> new file mode 100644
> index 0000000..a234a0c
> --- /dev/null
> +++ b/drivers/watchdog/imx_sc_wdt.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018-2019 NXP.
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/io.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/watchdog.h>
> +
> +#define DEFAULT_TIMEOUT 60
> +/*
> + * Software timer tick implemented in scfw side, support 10ms to 0xffffffff ms
> + * in theory, but for normal case, 1s~128s is enough, you can change this max
> + * value in case it's not enough.
> + */
> +#define MAX_TIMEOUT 128
> +
> +#define IMX_SIP_TIMER 0xC2000002
> +#define IMX_SIP_TIMER_START_WDOG 0x01
> +#define IMX_SIP_TIMER_STOP_WDOG 0x02
> +#define IMX_SIP_TIMER_SET_WDOG_ACT 0x03
> +#define IMX_SIP_TIMER_PING_WDOG 0x04
> +#define IMX_SIP_TIMER_SET_TIMEOUT_WDOG 0x05
> +#define IMX_SIP_TIMER_GET_WDOG_STAT 0x06
> +#define IMX_SIP_TIMER_SET_PRETIME_WDOG 0x07
> +
> +#define SC_TIMER_WDOG_ACTION_PARTITION 0
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0000);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static unsigned int timeout = DEFAULT_TIMEOUT;
Should not be initialized.
> +module_param(timeout, uint, 0000);
> +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
> + __MODULE_STRING(DEFAULT_TIMEOUT) ")");
> +
This suggests that you want to use that timeout. But that isn't the case.
> +static int imx_sc_wdt_ping(struct watchdog_device *wdog)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_PING_WDOG,
> + 0, 0, 0, 0, 0, 0, &res);
> +
> + return res.a0;
> +}
> +
> +static int imx_sc_wdt_start(struct watchdog_device *wdog)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_START_WDOG,
> + 0, 0, 0, 0, 0, 0, &res);
> + if (res.a0)
> + return res.a0;
> +
> + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_SET_WDOG_ACT,
> + SC_TIMER_WDOG_ACTION_PARTITION,
> + 0, 0, 0, 0, 0, &res);
> + return res.a0;
> +}
> +
> +static int imx_sc_wdt_stop(struct watchdog_device *wdog)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_STOP_WDOG,
> + 0, 0, 0, 0, 0, 0, &res);
> +
> + return res.a0;
> +}
> +
> +static int imx_sc_wdt_set_timeout(struct watchdog_device *wdog,
> + unsigned int timeout)
> +{
> + struct arm_smccc_res res;
> +
> + wdog->timeout = timeout;
> + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_SET_TIMEOUT_WDOG,
> + timeout * 1000, 0, 0, 0, 0, 0, &res);
> +
> + return res.a0;
> +}
> +
> +static const struct watchdog_ops imx_sc_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = imx_sc_wdt_start,
> + .stop = imx_sc_wdt_stop,
> + .ping = imx_sc_wdt_ping,
> + .set_timeout = imx_sc_wdt_set_timeout,
> +};
> +
> +static const struct watchdog_info imx_sc_wdt_info = {
> + .identity = "i.MX SC watchdog timer",
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> + WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT,
> +};
> +
> +static int imx_sc_wdt_probe(struct platform_device *pdev)
> +{
> + struct watchdog_device *imx_sc_wdd;
> + int ret;
> +
> + imx_sc_wdd = devm_kzalloc(&pdev->dev, sizeof(*imx_sc_wdd), GFP_KERNEL);
> + if (!imx_sc_wdd)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, imx_sc_wdd);
> +
> + imx_sc_wdd->info = &imx_sc_wdt_info;
> + imx_sc_wdd->ops = &imx_sc_wdt_ops;
> + imx_sc_wdd->min_timeout = 1;
> + imx_sc_wdd->max_timeout = MAX_TIMEOUT;
> + imx_sc_wdd->parent = &pdev->dev;
imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
> +
> + ret = watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT, &pdev->dev);
As written this doesn't make sense: It will set the timeout to the default,
which is always valid, making the error check unnecessary. I assume you meant
the above plus
ret = watchdog_init_timeout(imx_sc_wdd, timeout, &pdev->dev);
By not pre=initializing 'timeout', the watchdog core will try to read the
configured timeout from devicetree. If it is not specified, it will keep
the default timeout which should be set explicitly as suggested above.
With that, the error check makes some sense, though it is unusual to
bail out if an out-of-range timeout was specified. But that is your call.
Again, sorry for not noticing this earlier.
Guenter
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to init the wdog timeout:%d\n",
> + ret);
> + return ret;
> + }
> +
> + watchdog_stop_on_reboot(imx_sc_wdd);
> + watchdog_stop_on_unregister(imx_sc_wdd);
> +
> + ret = devm_watchdog_register_device(&pdev->dev, imx_sc_wdd);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register watchdog device\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id imx_sc_wdt_dt_ids[] = {
> + { .compatible = "fsl,imx8qxp-sc-wdt", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_sc_wdt_dt_ids);
> +
> +static int __maybe_unused imx_sc_wdt_suspend(struct device *dev)
> +{
> + struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
> +
> + if (watchdog_active(imx_sc_wdd))
> + imx_sc_wdt_stop(imx_sc_wdd);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused imx_sc_wdt_resume(struct device *dev)
> +{
> + struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
> +
> + if (watchdog_active(imx_sc_wdd))
> + imx_sc_wdt_start(imx_sc_wdd);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(imx_sc_wdt_pm_ops,
> + imx_sc_wdt_suspend, imx_sc_wdt_resume);
> +
> +static struct platform_driver imx_sc_wdt_driver = {
> + .probe = imx_sc_wdt_probe,
> + .driver = {
> + .name = "imx-sc-wdt",
> + .of_match_table = imx_sc_wdt_dt_ids,
> + .pm = &imx_sc_wdt_pm_ops,
> + },
> +};
> +
> +module_platform_driver(imx_sc_wdt_driver);
> +
> +MODULE_AUTHOR("Robin Gong <[email protected]>");
> +MODULE_DESCRIPTION("NXP i.MX system controller watchdog driver");
> +MODULE_LICENSE("GPL v2");
>
Hi, Guenter
Best Regards!
Anson Huang
> -----Original Message-----
> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
> Roeck
> Sent: 2019年2月24日 11:29
> To: Anson Huang <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Aisheng Dong
> <[email protected]>; [email protected]; Daniel Baluta
> <[email protected]>; Andy Gross <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected]
> Cc: dl-linux-imx <[email protected]>
> Subject: Re: [PATCH RESEND V2 2/4] watchdog: imx_sc: Add i.MX system
> controller watchdog support
>
> On 2/17/19 10:53 PM, Anson Huang wrote:
> > i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
> > inside, the system controller is in charge of controlling power, clock
> > and watchdog etc..
> >
> > This patch adds i.MX system controller watchdog driver support,
> > watchdog operation needs to be done in secure EL3 mode via
> > ARM-Trusted-Firmware, using SMC call, CPU will trap into
> > ARM-Trusted-Firmware and then it will request system controller to do
> > watchdog operation via IPC.
> >
> > Signed-off-by: Anson Huang <[email protected]>
>
> Sorry, but I have to take back my Reviewed-by: Comments below.
> Sorry, I completely missed that earlier.
>
> > ---
> > Changes since V1:
> > - use watchdog_active() instead of comparing watchdog status directly;
> > - use devm_kzalloc instead of using static variable;
> > - remove unnecessary wdog set driver data in .probe;
> > - use watchdog_stop_on_unregister() instead of .remove callback;
> > - use watchdog_stop_on_reboot() instead of .shutdown callback;
> > - use SIMPLE_DEV_PM_OPS() to simply the PM code.
> > ---
> > drivers/watchdog/Kconfig | 13 +++
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/imx_sc_wdt.c | 185
> ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 199 insertions(+)
> > create mode 100644 drivers/watchdog/imx_sc_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > 65c3c42..5c5b8ba 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -625,6 +625,19 @@ config IMX2_WDT
> > To compile this driver as a module, choose M here: the
> > module will be called imx2_wdt.
> >
> > +config IMX_SC_WDT
> > + tristate "IMX SC Watchdog"
> > + depends on ARCH_MXC || COMPILE_TEST
> > + select WATCHDOG_CORE
> > + help
> > + This is the driver for the system controller watchdog
> > + on the NXP i.MX SoCs with system controller inside.
> > + If you have one of these processors and wish to have
> > + watchdog support enabled, say Y, otherwise say N.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called imx_sc_wdt.
> > +
> > config UX500_WATCHDOG
> > tristate "ST-Ericsson Ux500 watchdog"
> > depends on MFD_DB8500_PRCMU
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 4e78a8c..0c9da63 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -68,6 +68,7 @@ obj-$(CONFIG_NUC900_WATCHDOG) +=
> nuc900_wdt.o
> > obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
> > obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
> > obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
> > +obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
> > obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
> > obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
> > obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o diff --git
> > a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c new
> > file mode 100644 index 0000000..a234a0c
> > --- /dev/null
> > +++ b/drivers/watchdog/imx_sc_wdt.c
> > @@ -0,0 +1,185 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + */
> > +
> > +#include <linux/arm-smccc.h>
> > +#include <linux/io.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reboot.h>
> > +#include <linux/watchdog.h>
> > +
> > +#define DEFAULT_TIMEOUT 60
> > +/*
> > + * Software timer tick implemented in scfw side, support 10ms to
> > +0xffffffff ms
> > + * in theory, but for normal case, 1s~128s is enough, you can change
> > +this max
> > + * value in case it's not enough.
> > + */
> > +#define MAX_TIMEOUT 128
> > +
> > +#define IMX_SIP_TIMER 0xC2000002
> > +#define IMX_SIP_TIMER_START_WDOG 0x01
> > +#define IMX_SIP_TIMER_STOP_WDOG 0x02
> > +#define IMX_SIP_TIMER_SET_WDOG_ACT 0x03
> > +#define IMX_SIP_TIMER_PING_WDOG 0x04
> > +#define IMX_SIP_TIMER_SET_TIMEOUT_WDOG 0x05
> > +#define IMX_SIP_TIMER_GET_WDOG_STAT 0x06
> > +#define IMX_SIP_TIMER_SET_PRETIME_WDOG 0x07
> > +
> > +#define SC_TIMER_WDOG_ACTION_PARTITION 0
> > +
> > +static bool nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout,
> > +bool, 0000); MODULE_PARM_DESC(nowayout, "Watchdog cannot be
> stopped
> > +once started (default="
> > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> > +
> > +static unsigned int timeout = DEFAULT_TIMEOUT;
>
> Should not be initialized.
>
> > +module_param(timeout, uint, 0000);
> > +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
> > + __MODULE_STRING(DEFAULT_TIMEOUT) ")");
> > +
>
> This suggests that you want to use that timeout. But that isn't the case.
You are correct, I should have used this module parameter when setting timeout;
>
>
>
>
> > +static int imx_sc_wdt_ping(struct watchdog_device *wdog) {
> > + struct arm_smccc_res res;
> > +
> > + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_PING_WDOG,
> > + 0, 0, 0, 0, 0, 0, &res);
> > +
> > + return res.a0;
> > +}
> > +
> > +static int imx_sc_wdt_start(struct watchdog_device *wdog) {
> > + struct arm_smccc_res res;
> > +
> > + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_START_WDOG,
> > + 0, 0, 0, 0, 0, 0, &res);
> > + if (res.a0)
> > + return res.a0;
> > +
> > + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_SET_WDOG_ACT,
> > + SC_TIMER_WDOG_ACTION_PARTITION,
> > + 0, 0, 0, 0, 0, &res);
> > + return res.a0;
> > +}
> > +
> > +static int imx_sc_wdt_stop(struct watchdog_device *wdog) {
> > + struct arm_smccc_res res;
> > +
> > + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_STOP_WDOG,
> > + 0, 0, 0, 0, 0, 0, &res);
> > +
> > + return res.a0;
> > +}
> > +
> > +static int imx_sc_wdt_set_timeout(struct watchdog_device *wdog,
> > + unsigned int timeout)
> > +{
> > + struct arm_smccc_res res;
> > +
> > + wdog->timeout = timeout;
> > + arm_smccc_smc(IMX_SIP_TIMER,
> IMX_SIP_TIMER_SET_TIMEOUT_WDOG,
> > + timeout * 1000, 0, 0, 0, 0, 0, &res);
> > +
> > + return res.a0;
> > +}
> > +
> > +static const struct watchdog_ops imx_sc_wdt_ops = {
> > + .owner = THIS_MODULE,
> > + .start = imx_sc_wdt_start,
> > + .stop = imx_sc_wdt_stop,
> > + .ping = imx_sc_wdt_ping,
> > + .set_timeout = imx_sc_wdt_set_timeout, };
> > +
> > +static const struct watchdog_info imx_sc_wdt_info = {
> > + .identity = "i.MX SC watchdog timer",
> > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> > + WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT, };
> > +
> > +static int imx_sc_wdt_probe(struct platform_device *pdev) {
> > + struct watchdog_device *imx_sc_wdd;
> > + int ret;
> > +
> > + imx_sc_wdd = devm_kzalloc(&pdev->dev, sizeof(*imx_sc_wdd),
> GFP_KERNEL);
> > + if (!imx_sc_wdd)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, imx_sc_wdd);
> > +
> > + imx_sc_wdd->info = &imx_sc_wdt_info;
> > + imx_sc_wdd->ops = &imx_sc_wdt_ops;
> > + imx_sc_wdd->min_timeout = 1;
> > + imx_sc_wdd->max_timeout = MAX_TIMEOUT;
> > + imx_sc_wdd->parent = &pdev->dev;
>
> imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
>
> > +
> > + ret = watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT,
> > +&pdev->dev);
>
> As written this doesn't make sense: It will set the timeout to the default,
> which is always valid, making the error check unnecessary. I assume you
> meant the above plus
> ret = watchdog_init_timeout(imx_sc_wdd, timeout, &pdev->dev);
>
> By not pre=initializing 'timeout', the watchdog core will try to read the
> configured timeout from devicetree. If it is not specified, it will keep the
> default timeout which should be set explicitly as suggested above.
>
> With that, the error check makes some sense, though it is unusual to bail out
> if an out-of-range timeout was specified. But that is your call.
>
> Again, sorry for not noticing this earlier.
Thanks for your detail review, in V3 patch series, I follow the suggestion to make
the timeout value to be default first, then try to set the watchdog timeout using
module parameter, if it is invalid, common driver will try to find it from dts node,
if return value is error, it will use default value set and NOT return error during probe.
This makes the error check making sense and NOT break watchdog driver probe even
if the module parameter "timeout" is invalid. Please help review V3 patch series.
Thanks,
Anson
>
> Guenter
>
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to init the wdog timeout:%d\n",
> > + ret);
> > + return ret;
> > + }
> > +
> > + watchdog_stop_on_reboot(imx_sc_wdd);
> > + watchdog_stop_on_unregister(imx_sc_wdd);
> > +
> > + ret = devm_watchdog_register_device(&pdev->dev, imx_sc_wdd);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to register watchdog device\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id imx_sc_wdt_dt_ids[] = {
> > + { .compatible = "fsl,imx8qxp-sc-wdt", },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx_sc_wdt_dt_ids);
> > +
> > +static int __maybe_unused imx_sc_wdt_suspend(struct device *dev) {
> > + struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
> > +
> > + if (watchdog_active(imx_sc_wdd))
> > + imx_sc_wdt_stop(imx_sc_wdd);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused imx_sc_wdt_resume(struct device *dev) {
> > + struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
> > +
> > + if (watchdog_active(imx_sc_wdd))
> > + imx_sc_wdt_start(imx_sc_wdd);
> > +
> > + return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(imx_sc_wdt_pm_ops,
> > + imx_sc_wdt_suspend, imx_sc_wdt_resume);
> > +
> > +static struct platform_driver imx_sc_wdt_driver = {
> > + .probe = imx_sc_wdt_probe,
> > + .driver = {
> > + .name = "imx-sc-wdt",
> > + .of_match_table = imx_sc_wdt_dt_ids,
> > + .pm = &imx_sc_wdt_pm_ops,
> > + },
> > +};
> > +
> > +module_platform_driver(imx_sc_wdt_driver);
> > +
> > +MODULE_AUTHOR("Robin Gong <[email protected]>");
> > +MODULE_DESCRIPTION("NXP i.MX system controller watchdog driver");
> > +MODULE_LICENSE("GPL v2");
> >
Hi, Guenter
Best Regards!
Anson Huang
> -----Original Message-----
> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
> Roeck
> Sent: 2019年2月24日 11:20
> To: Anson Huang <[email protected]>; Rob Herring <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Aisheng Dong <[email protected]>; [email protected]; Daniel
> Baluta <[email protected]>; Andy Gross <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>
> Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog
> binding
>
> On 2/23/19 7:04 PM, Anson Huang wrote:
> > Hi, Guenter/Rob
> >
> > Best Regards!
> > Anson Huang
> >
> >> -----Original Message-----
> >> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
> >> Roeck
> >> Sent: 2019年2月24日 1:08
> >> To: Rob Herring <[email protected]>; Anson Huang
> <[email protected]>
> >> Cc: [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected]; wim@linux-
> watchdog.org;
> >> Aisheng Dong <[email protected]>; [email protected]; Daniel
> >> Baluta <[email protected]>; Andy Gross <[email protected]>;
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; linux-arm- [email protected];
> >> [email protected]; dl-linux-imx <[email protected]>
> >> Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add
> >> watchdog binding
> >>
> >> On 2/22/19 11:52 AM, Rob Herring wrote:
> >>> On Mon, Feb 18, 2019 at 06:53:48AM +0000, Anson Huang wrote:
> >>>> Add i.MX8QXP system controller watchdog binding.
> >>>>
> >>>> Signed-off-by: Anson Huang <[email protected]>
> >>>> ---
> >>>> Changes since V1:
> >>>> - update dts node name to "watchdog";
> >>>> ---
> >>>> Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 10
> >> ++++++++++
> >>>> 1 file changed, 10 insertions(+)
> >>>>
> >>>> diff --git
> >>>> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >>>> b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >>>> index 4b79751..f388ec6 100644
> >>>> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >>>> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >>>> @@ -136,6 +136,12 @@ Required properties:
> >>>> resource id for thermal driver to get temperature
> >> via
> >>>> SCU IPC.
> >>>>
> >>>> +Watchdog bindings based on SCU Message Protocol
> >>>> +------------------------------------------------------------
> >>>> +
> >>>> +Required properties:
> >>>> +- compatible: should be "fsl,imx8qxp-sc-wdt";
> >>>> +
> >>>> Example (imx8qxp):
> >>>> -------------
> >>>> lsio_mu1: mailbox@5d1c0000 {
> >>>> @@ -188,6 +194,10 @@ firmware {
> >>>> tsens-num = <1>;
> >>>> #thermal-sensor-cells = <1>;
> >>>> };
> >>>> +
> >>>> + watchdog: watchdog {
> >>>> + compatible = "fsl,imx8qxp-sc-wdt";
> >>>
> >>> As-is, there's no reason for this to be in DT. The parent node's
> >>> driver can instantiate the wdog.
> >>>
> >>
> >> As the driver is currently written, you are correct, since it doesn't
> >> accept watchdog timeout configuration through devicetree.
> >>
> >> Question is if that is intended. Is it ?
> >
> > I am a little confused, do you mean we no need to have "watchdog" node
> in side "scu"
> > node? Or we need to modify the watchdog node's compatible string to "
> > fsl,imx-sc-wdt" to make it more generic for other platforms? If yes, I can
> resend the patch series to modify it.
> >
>
> I think Rob suggested that the SCU parent driver should instantiate the
> watchdog without explicit watchdog node. That would be possible, but it
> currently uses
> devm_of_platform_populate() to do the instantiation, and changing that
> would be a mess. Besides, it does sem to me that your suggested node would
> describe the hardware, so I am not sure I understand the reasoning.
>
> For my part I referred to
> watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT, &pdev-
> >dev); in the driver, which guarantees that the timeout property will not be
> used to set the timeout. A more common implementation would have been
>
> imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
> ret = watchdog_init_timeout(imx_sc_wdd, timeout, &pdev->dev);
>
> where 'timeout' is the module parameter. Which is actually not used in your
> driver.
> Hmm ... I wasn't careful enough with my review. The timeout initialization as-
> is doesn't make sense. I'll comment on that in the patch.
I understand now, in our cases, I would still prefer to have watchdog node under
the SCU parent node, since there could be other property setting difference between
different i.MX platforms with system controller watchdog inside, using the SCU node
to instantiate makes us a little confused about the watchdog, so if it is NOT that critical,
I think we should keep watchdog node. But to make the watchdog driver more generic
for other i.MX platforms, I changed the compatible string to "fsl,imx-sc-wdt" in driver, and
each SoC should has it as fallback if it can reuse this watchdog driver.
Thanks,
Anson
>
> Guenter
>
> > Anson.
> >
> >>
> >> Thanks,
> >> Guenter
> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
> Roeck>
> On 2/22/19 11:52 AM, Rob Herring wrote:
> > On Mon, Feb 18, 2019 at 06:53:48AM +0000, Anson Huang wrote:
> >> Add i.MX8QXP system controller watchdog binding.
> >>
> >> Signed-off-by: Anson Huang <[email protected]>
> >> ---
> >> Changes since V1:
> >> - update dts node name to "watchdog";
> >> ---
> >> Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 10
> ++++++++++
> >> 1 file changed, 10 insertions(+)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >> b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >> index 4b79751..f388ec6 100644
> >> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >> @@ -136,6 +136,12 @@ Required properties:
> >> resource id for thermal driver to get temperature via
> >> SCU IPC.
> >>
> >> +Watchdog bindings based on SCU Message Protocol
> >> +------------------------------------------------------------
> >> +
> >> +Required properties:
> >> +- compatible: should be "fsl,imx8qxp-sc-wdt";
> >> +
> >> Example (imx8qxp):
> >> -------------
> >> lsio_mu1: mailbox@5d1c0000 {
> >> @@ -188,6 +194,10 @@ firmware {
> >> tsens-num = <1>;
> >> #thermal-sensor-cells = <1>;
> >> };
> >> +
> >> + watchdog: watchdog {
> >> + compatible = "fsl,imx8qxp-sc-wdt";
> >
> > As-is, there's no reason for this to be in DT. The parent node's
> > driver can instantiate the wdog.
> >
>
> As the driver is currently written, you are correct, since it doesn't accept
> watchdog timeout configuration through devicetree.
>
> Question is if that is intended. Is it ?
>
Per my understanding, if the protocol is unlike to change, it can be instantiated
by portent node's driver.
Hope Rob can help give a more general explanation.
Regards
Dong Aisheng
> Thanks,
> Guenter
On Sun, Feb 24, 2019 at 8:26 PM Anson Huang <[email protected]> wrote:
>
> Hi, Guenter
>
> Best Regards!
> Anson Huang
>
> > -----Original Message-----
> > From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
> > Roeck
> > Sent: 2019年2月24日 11:20
> > To: Anson Huang <[email protected]>; Rob Herring <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > Aisheng Dong <[email protected]>; [email protected]; Daniel
> > Baluta <[email protected]>; Andy Gross <[email protected]>;
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; linux-arm-
> > [email protected]; [email protected]; dl-linux-imx
> > <[email protected]>
> > Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog
> > binding
> >
> > On 2/23/19 7:04 PM, Anson Huang wrote:
> > > Hi, Guenter/Rob
> > >
> > > Best Regards!
> > > Anson Huang
> > >
> > >> -----Original Message-----
> > >> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
> > >> Roeck
> > >> Sent: 2019年2月24日 1:08
> > >> To: Rob Herring <[email protected]>; Anson Huang
> > <[email protected]>
> > >> Cc: [email protected]; [email protected];
> > >> [email protected]; [email protected]; [email protected];
> > >> [email protected]; [email protected]; wim@linux-
> > watchdog.org;
> > >> Aisheng Dong <[email protected]>; [email protected]; Daniel
> > >> Baluta <[email protected]>; Andy Gross <[email protected]>;
> > >> [email protected]; [email protected]; [email protected];
> > >> [email protected]; [email protected];
> > >> [email protected]; [email protected];
> > >> [email protected]; [email protected];
> > >> [email protected]; linux-arm- [email protected];
> > >> [email protected]; dl-linux-imx <[email protected]>
> > >> Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add
> > >> watchdog binding
> > >>
> > >> On 2/22/19 11:52 AM, Rob Herring wrote:
> > >>> On Mon, Feb 18, 2019 at 06:53:48AM +0000, Anson Huang wrote:
> > >>>> Add i.MX8QXP system controller watchdog binding.
> > >>>>
> > >>>> Signed-off-by: Anson Huang <[email protected]>
> > >>>> ---
> > >>>> Changes since V1:
> > >>>> - update dts node name to "watchdog";
> > >>>> ---
> > >>>> Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 10
> > >> ++++++++++
> > >>>> 1 file changed, 10 insertions(+)
> > >>>>
> > >>>> diff --git
> > >>>> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > >>>> b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > >>>> index 4b79751..f388ec6 100644
> > >>>> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > >>>> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > >>>> @@ -136,6 +136,12 @@ Required properties:
> > >>>> resource id for thermal driver to get temperature
> > >> via
> > >>>> SCU IPC.
> > >>>>
> > >>>> +Watchdog bindings based on SCU Message Protocol
> > >>>> +------------------------------------------------------------
> > >>>> +
> > >>>> +Required properties:
> > >>>> +- compatible: should be "fsl,imx8qxp-sc-wdt";
> > >>>> +
> > >>>> Example (imx8qxp):
> > >>>> -------------
> > >>>> lsio_mu1: mailbox@5d1c0000 {
> > >>>> @@ -188,6 +194,10 @@ firmware {
> > >>>> tsens-num = <1>;
> > >>>> #thermal-sensor-cells = <1>;
> > >>>> };
> > >>>> +
> > >>>> + watchdog: watchdog {
> > >>>> + compatible = "fsl,imx8qxp-sc-wdt";
> > >>>
> > >>> As-is, there's no reason for this to be in DT. The parent node's
> > >>> driver can instantiate the wdog.
> > >>>
> > >>
> > >> As the driver is currently written, you are correct, since it doesn't
> > >> accept watchdog timeout configuration through devicetree.
> > >>
> > >> Question is if that is intended. Is it ?
> > >
> > > I am a little confused, do you mean we no need to have "watchdog" node
> > in side "scu"
> > > node? Or we need to modify the watchdog node's compatible string to "
> > > fsl,imx-sc-wdt" to make it more generic for other platforms? If yes, I can
> > resend the patch series to modify it.
> > >
> >
> > I think Rob suggested that the SCU parent driver should instantiate the
> > watchdog without explicit watchdog node. That would be possible, but it
> > currently uses
> > devm_of_platform_populate() to do the instantiation, and changing that
> > would be a mess. Besides, it does sem to me that your suggested node would
> > describe the hardware, so I am not sure I understand the reasoning.
It would just be a call to create a platform device instead. How is that a mess?
It's describing firmware. We have DT for describing h/w we've failed
to make discoverable. We should not repeat that and just describe
firmware in DT. Make the firmware discoverable! Though there are cases
like firmware provided clocks where we still need something in DT, but
this is not one of them.
> >
> > For my part I referred to
> > watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT, &pdev-
> > >dev); in the driver, which guarantees that the timeout property will not be
> > used to set the timeout. A more common implementation would have been
> >
> > imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
> > ret = watchdog_init_timeout(imx_sc_wdd, timeout, &pdev->dev);
> >
> > where 'timeout' is the module parameter. Which is actually not used in your
> > driver.
> > Hmm ... I wasn't careful enough with my review. The timeout initialization as-
> > is doesn't make sense. I'll comment on that in the patch.
>
> I understand now, in our cases, I would still prefer to have watchdog node under
> the SCU parent node, since there could be other property setting difference between
> different i.MX platforms with system controller watchdog inside, using the SCU node
> to instantiate makes us a little confused about the watchdog, so if it is NOT that critical,
> I think we should keep watchdog node. But to make the watchdog driver more generic
> for other i.MX platforms, I changed the compatible string to "fsl,imx-sc-wdt" in driver, and
> each SoC should has it as fallback if it can reuse this watchdog driver.
You handle differences between SoCs by having specific compatibles. So
"fsl,imx-sc-wdt" moves in the wrong direction assuming we have a node
in the first place.
Rob
On Tue, Feb 26, 2019 at 03:34:12PM -0600, Rob Herring wrote:
> > >
> > > I think Rob suggested that the SCU parent driver should instantiate the
> > > watchdog without explicit watchdog node. That would be possible, but it
> > > currently uses
> > > devm_of_platform_populate() to do the instantiation, and changing that
> > > would be a mess. Besides, it does sem to me that your suggested node would
> > > describe the hardware, so I am not sure I understand the reasoning.
>
> It would just be a call to create a platform device instead. How is that a mess?
>
> It's describing firmware. We have DT for describing h/w we've failed
> to make discoverable. We should not repeat that and just describe
> firmware in DT. Make the firmware discoverable! Though there are cases
> like firmware provided clocks where we still need something in DT, but
> this is not one of them.
>
It requires extra code where an added DT node would accomplish the same.
It requires a mix of DT nodes for existing devices plus extra code for
newly added devices. To me that looks like a revert to old platform code,
which was replaced with DT descriptions over the last several years.
But then if that is where things are going, who am I to argue.
Guenter
Hi, Rob
Sorry to bring back this topic again about whether to put "imx-sc-wdt" node inside DT's SCU node, after some discussion with my team, there is case of virtualization, disabling "imx-sc-wdt" for Linux OS, while enable it for Android OS running together on same SoC, then Linux needs to disable watchdog from its DTB while Android can enable it. For such kind of scenario, do you think it is reasonable to have "imx-sc-wdt" node inside SCU node in DT? We do NOT have good idea for this scenario if imx-sc-wdt is added as built-in platform device inside SCU driver.
Best Regards!
Anson Huang
> -----Original Message-----
> From: Rob Herring [mailto:[email protected]]
> Sent: 2019年2月27日 5:34
> To: Anson Huang <[email protected]>
> Cc: Guenter Roeck <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Andy Gross <[email protected]>; dl-
> linux-imx <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Daniel Baluta
> <[email protected]>; [email protected]; Aisheng
> Dong <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog
> binding
>
> On Sun, Feb 24, 2019 at 8:26 PM Anson Huang <[email protected]>
> wrote:
> >
> > Hi, Guenter
> >
> > Best Regards!
> > Anson Huang
> >
> > > -----Original Message-----
> > > From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
> > > Roeck
> > > Sent: 2019年2月24日 11:20
> > > To: Anson Huang <[email protected]>; Rob Herring
> <[email protected]>
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; Aisheng Dong <[email protected]>;
> > > [email protected]; Daniel Baluta <[email protected]>; Andy
> > > Gross <[email protected]>;
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; linux-arm- [email protected];
> > > [email protected]; dl-linux-imx <[email protected]>
> > > Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add
> > > watchdog binding
> > >
> > > On 2/23/19 7:04 PM, Anson Huang wrote:
> > > > Hi, Guenter/Rob
> > > >
> > > > Best Regards!
> > > > Anson Huang
> > > >
> > > >> -----Original Message-----
> > > >> From: Guenter Roeck [mailto:[email protected]] On Behalf Of
> > > >> Guenter Roeck
> > > >> Sent: 2019年2月24日 1:08
> > > >> To: Rob Herring <[email protected]>; Anson Huang
> > > <[email protected]>
> > > >> Cc: [email protected]; [email protected];
> > > >> [email protected]; [email protected];
> > > >> [email protected]; [email protected];
> [email protected];
> > > >> wim@linux-
> > > watchdog.org;
> > > >> Aisheng Dong <[email protected]>; [email protected];
> > > >> Daniel Baluta <[email protected]>; Andy Gross
> > > >> <[email protected]>;
> > > >> [email protected]; [email protected]; [email protected];
> > > >> [email protected]; [email protected];
> > > >> [email protected]; [email protected];
> > > >> [email protected]; [email protected];
> > > >> [email protected]; linux-arm-
> > > >> [email protected]; [email protected];
> > > >> dl-linux-imx <[email protected]>
> > > >> Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add
> > > >> watchdog binding
> > > >>
> > > >> On 2/22/19 11:52 AM, Rob Herring wrote:
> > > >>> On Mon, Feb 18, 2019 at 06:53:48AM +0000, Anson Huang wrote:
> > > >>>> Add i.MX8QXP system controller watchdog binding.
> > > >>>>
> > > >>>> Signed-off-by: Anson Huang <[email protected]>
> > > >>>> ---
> > > >>>> Changes since V1:
> > > >>>> - update dts node name to "watchdog";
> > > >>>> ---
> > > >>>> Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > >>>> | 10
> > > >> ++++++++++
> > > >>>> 1 file changed, 10 insertions(+)
> > > >>>>
> > > >>>> diff --git
> > > >>>> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > >>>> b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > >>>> index 4b79751..f388ec6 100644
> > > >>>> ---
> > > >>>> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > >>>> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.t
> > > >>>> +++ xt
> > > >>>> @@ -136,6 +136,12 @@ Required properties:
> > > >>>> resource id for thermal driver to
> > > >>>> get temperature
> > > >> via
> > > >>>> SCU IPC.
> > > >>>>
> > > >>>> +Watchdog bindings based on SCU Message Protocol
> > > >>>> +------------------------------------------------------------
> > > >>>> +
> > > >>>> +Required properties:
> > > >>>> +- compatible: should be "fsl,imx8qxp-sc-wdt";
> > > >>>> +
> > > >>>> Example (imx8qxp):
> > > >>>> -------------
> > > >>>> lsio_mu1: mailbox@5d1c0000 { @@ -188,6 +194,10 @@ firmware
> {
> > > >>>> tsens-num = <1>;
> > > >>>> #thermal-sensor-cells = <1>;
> > > >>>> };
> > > >>>> +
> > > >>>> + watchdog: watchdog {
> > > >>>> + compatible = "fsl,imx8qxp-sc-wdt";
> > > >>>
> > > >>> As-is, there's no reason for this to be in DT. The parent node's
> > > >>> driver can instantiate the wdog.
> > > >>>
> > > >>
> > > >> As the driver is currently written, you are correct, since it
> > > >> doesn't accept watchdog timeout configuration through devicetree.
> > > >>
> > > >> Question is if that is intended. Is it ?
> > > >
> > > > I am a little confused, do you mean we no need to have "watchdog"
> > > > node
> > > in side "scu"
> > > > node? Or we need to modify the watchdog node's compatible string to "
> > > > fsl,imx-sc-wdt" to make it more generic for other platforms? If
> > > > yes, I can
> > > resend the patch series to modify it.
> > > >
> > >
> > > I think Rob suggested that the SCU parent driver should instantiate
> > > the watchdog without explicit watchdog node. That would be possible,
> > > but it currently uses
> > > devm_of_platform_populate() to do the instantiation, and changing
> > > that would be a mess. Besides, it does sem to me that your suggested
> > > node would describe the hardware, so I am not sure I understand the
> reasoning.
>
> It would just be a call to create a platform device instead. How is that a mess?
>
> It's describing firmware. We have DT for describing h/w we've failed to make
> discoverable. We should not repeat that and just describe firmware in DT.
> Make the firmware discoverable! Though there are cases like firmware
> provided clocks where we still need something in DT, but this is not one of
> them.
>
> > >
> > > For my part I referred to
> > > watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT, &pdev-
> > > >dev); in the driver, which guarantees that the timeout property
> > > >will not be
> > > used to set the timeout. A more common implementation would have
> > > been
> > >
> > > imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
> > > ret = watchdog_init_timeout(imx_sc_wdd, timeout, &pdev->dev);
> > >
> > > where 'timeout' is the module parameter. Which is actually not used
> > > in your driver.
> > > Hmm ... I wasn't careful enough with my review. The timeout
> > > initialization as- is doesn't make sense. I'll comment on that in the patch.
> >
> > I understand now, in our cases, I would still prefer to have watchdog
> > node under the SCU parent node, since there could be other property
> > setting difference between different i.MX platforms with system
> > controller watchdog inside, using the SCU node to instantiate makes us
> > a little confused about the watchdog, so if it is NOT that critical, I
> > think we should keep watchdog node. But to make the watchdog driver
> > more generic for other i.MX platforms, I changed the compatible string to
> "fsl,imx-sc-wdt" in driver, and each SoC should has it as fallback if it can
> reuse this watchdog driver.
>
> You handle differences between SoCs by having specific compatibles. So
> "fsl,imx-sc-wdt" moves in the wrong direction assuming we have a node in
> the first place.
>
> Rob
Hi Rob,
> > > I think Rob suggested that the SCU parent driver should instantiate
> > > the watchdog without explicit watchdog node. That would be possible,
> > > but it currently uses
> > > devm_of_platform_populate() to do the instantiation, and changing
> > > that would be a mess. Besides, it does sem to me that your suggested
> > > node would describe the hardware, so I am not sure I understand the
> reasoning.
>
> It would just be a call to create a platform device instead. How is that a mess?
>
> It's describing firmware. We have DT for describing h/w we've failed to make
> discoverable. We should not repeat that and just describe firmware in DT.
> Make the firmware discoverable! Though there are cases like firmware
> provided clocks where we still need something in DT, but this is not one of
> them.
>
The watchdog node here in question actually is not using SCU firmware call.
Due to security requirement by SCU, watchdog can only be accessed in
security mode, for IMX case, via ARM Trust Firmware. That means the
watchdog used in Linux actually is using ARM SMC call and does not
depend SCU driver. So It would be strange for SCU driver to instantiate it.
For this situation, do you think we can move watchdog out of scu node?
Maybe rename the compatible string like "fsl,imx8qxp-sip-watchdog"
because it's actually a watchdog serviced by ATF firmware.
Regards
Dong Aisheng
+Jens W
On Thu, Mar 7, 2019 at 6:22 AM Aisheng Dong <[email protected]> wrote:
>
> Hi Rob,
>
> > > > I think Rob suggested that the SCU parent driver should instantiate
> > > > the watchdog without explicit watchdog node. That would be possible,
> > > > but it currently uses
> > > > devm_of_platform_populate() to do the instantiation, and changing
> > > > that would be a mess. Besides, it does sem to me that your suggested
> > > > node would describe the hardware, so I am not sure I understand the
> > reasoning.
> >
> > It would just be a call to create a platform device instead. How is that a mess?
> >
> > It's describing firmware. We have DT for describing h/w we've failed to make
> > discoverable. We should not repeat that and just describe firmware in DT.
> > Make the firmware discoverable! Though there are cases like firmware
> > provided clocks where we still need something in DT, but this is not one of
> > them.
> >
>
> The watchdog node here in question actually is not using SCU firmware call.
> Due to security requirement by SCU, watchdog can only be accessed in
> security mode, for IMX case, via ARM Trust Firmware. That means the
> watchdog used in Linux actually is using ARM SMC call and does not
> depend SCU driver. So It would be strange for SCU driver to instantiate it.
>
> For this situation, do you think we can move watchdog out of scu node?
> Maybe rename the compatible string like "fsl,imx8qxp-sip-watchdog"
> because it's actually a watchdog serviced by ATF firmware.
Yes, but that creates more questions. What exactly does ATF talk to
for the watchdog? The SCU firmware?
Maybe ATF should define and provide a standard watchdog interface? It
is still a question of making the firmware discoverable rather than
trying to describe the firmware in DT.
Rob
Hi, Rob
Do you have any feedback about adding imx scu watchdog node in dts? Thanks.
Best Regards!
Anson Huang
> -----Original Message-----
> From: Anson Huang
> Sent: 2019年3月6日 22:45
> To: 'Rob Herring' <[email protected]>
> Cc: Guenter Roeck <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Andy Gross <[email protected]>; dl-
> linux-imx <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Daniel Baluta
> <[email protected]>; [email protected]; Aisheng
> Dong <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: RE: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog
> binding
>
> Hi, Rob
> Sorry to bring back this topic again about whether to put "imx-sc-
> wdt" node inside DT's SCU node, after some discussion with my team, there
> is case of virtualization, disabling "imx-sc-wdt" for Linux OS, while enable it
> for Android OS running together on same SoC, then Linux needs to disable
> watchdog from its DTB while Android can enable it. For such kind of scenario,
> do you think it is reasonable to have "imx-sc-wdt" node inside SCU node in
> DT? We do NOT have good idea for this scenario if imx-sc-wdt is added as
> built-in platform device inside SCU driver.
>
> Best Regards!
> Anson Huang
>
> > -----Original Message-----
> > From: Rob Herring [mailto:[email protected]]
> > Sent: 2019年2月27日 5:34
> > To: Anson Huang <[email protected]>
> > Cc: Guenter Roeck <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; Andy Gross <[email protected]>; dl-
> > linux-imx <[email protected]>; [email protected]; linux-
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; Daniel Baluta
> > <[email protected]>; [email protected]; Aisheng
> > Dong <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected]
> > Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog
> > binding
> >
> > On Sun, Feb 24, 2019 at 8:26 PM Anson Huang <[email protected]>
> > wrote:
> > >
> > > Hi, Guenter
> > >
> > > Best Regards!
> > > Anson Huang
> > >
> > > > -----Original Message-----
> > > > From: Guenter Roeck [mailto:[email protected]] On Behalf Of
> > > > Guenter Roeck
> > > > Sent: 2019年2月24日 11:20
> > > > To: Anson Huang <[email protected]>; Rob Herring
> > <[email protected]>
> > > > Cc: [email protected]; [email protected];
> > > > [email protected]; [email protected];
> [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]; Aisheng Dong <[email protected]>;
> > > > [email protected]; Daniel Baluta <[email protected]>;
> > > > Andy Gross <[email protected]>;
> > > > [email protected]; [email protected]; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]; linux-arm-
> > > > [email protected]; [email protected];
> > > > dl-linux-imx <[email protected]>
> > > > Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add
> > > > watchdog binding
> > > >
> > > > On 2/23/19 7:04 PM, Anson Huang wrote:
> > > > > Hi, Guenter/Rob
> > > > >
> > > > > Best Regards!
> > > > > Anson Huang
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: Guenter Roeck [mailto:[email protected]] On Behalf Of
> > > > >> Guenter Roeck
> > > > >> Sent: 2019年2月24日 1:08
> > > > >> To: Rob Herring <[email protected]>; Anson Huang
> > > > <[email protected]>
> > > > >> Cc: [email protected]; [email protected];
> > > > >> [email protected]; [email protected];
> > > > >> [email protected]; [email protected];
> > [email protected];
> > > > >> wim@linux-
> > > > watchdog.org;
> > > > >> Aisheng Dong <[email protected]>; [email protected];
> > > > >> Daniel Baluta <[email protected]>; Andy Gross
> > > > >> <[email protected]>;
> > > > >> [email protected]; [email protected]; [email protected];
> > > > >> [email protected]; [email protected];
> > > > >> [email protected]; [email protected];
> > > > >> [email protected]; [email protected];
> > > > >> [email protected]; linux-arm-
> > > > >> [email protected]; [email protected];
> > > > >> dl-linux-imx <[email protected]>
> > > > >> Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add
> > > > >> watchdog binding
> > > > >>
> > > > >> On 2/22/19 11:52 AM, Rob Herring wrote:
> > > > >>> On Mon, Feb 18, 2019 at 06:53:48AM +0000, Anson Huang wrote:
> > > > >>>> Add i.MX8QXP system controller watchdog binding.
> > > > >>>>
> > > > >>>> Signed-off-by: Anson Huang <[email protected]>
> > > > >>>> ---
> > > > >>>> Changes since V1:
> > > > >>>> - update dts node name to "watchdog";
> > > > >>>> ---
> > > > >>>>
> > > > >>>> Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > >>>> | 10
> > > > >> ++++++++++
> > > > >>>> 1 file changed, 10 insertions(+)
> > > > >>>>
> > > > >>>> diff --git
> > > > >>>> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > >>>> b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > >>>> index 4b79751..f388ec6 100644
> > > > >>>> ---
> > > > >>>> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > >>>> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu
> > > > >>>> +++ .t
> > > > >>>> +++ xt
> > > > >>>> @@ -136,6 +136,12 @@ Required properties:
> > > > >>>> resource id for thermal driver to
> > > > >>>> get temperature
> > > > >> via
> > > > >>>> SCU IPC.
> > > > >>>>
> > > > >>>> +Watchdog bindings based on SCU Message Protocol
> > > > >>>> +------------------------------------------------------------
> > > > >>>> +
> > > > >>>> +Required properties:
> > > > >>>> +- compatible: should be "fsl,imx8qxp-sc-wdt";
> > > > >>>> +
> > > > >>>> Example (imx8qxp):
> > > > >>>> -------------
> > > > >>>> lsio_mu1: mailbox@5d1c0000 { @@ -188,6 +194,10 @@
> firmware
> > {
> > > > >>>> tsens-num = <1>;
> > > > >>>> #thermal-sensor-cells = <1>;
> > > > >>>> };
> > > > >>>> +
> > > > >>>> + watchdog: watchdog {
> > > > >>>> + compatible = "fsl,imx8qxp-sc-wdt";
> > > > >>>
> > > > >>> As-is, there's no reason for this to be in DT. The parent
> > > > >>> node's driver can instantiate the wdog.
> > > > >>>
> > > > >>
> > > > >> As the driver is currently written, you are correct, since it
> > > > >> doesn't accept watchdog timeout configuration through devicetree.
> > > > >>
> > > > >> Question is if that is intended. Is it ?
> > > > >
> > > > > I am a little confused, do you mean we no need to have "watchdog"
> > > > > node
> > > > in side "scu"
> > > > > node? Or we need to modify the watchdog node's compatible string
> to "
> > > > > fsl,imx-sc-wdt" to make it more generic for other platforms? If
> > > > > yes, I can
> > > > resend the patch series to modify it.
> > > > >
> > > >
> > > > I think Rob suggested that the SCU parent driver should
> > > > instantiate the watchdog without explicit watchdog node. That
> > > > would be possible, but it currently uses
> > > > devm_of_platform_populate() to do the instantiation, and changing
> > > > that would be a mess. Besides, it does sem to me that your
> > > > suggested node would describe the hardware, so I am not sure I
> > > > understand the
> > reasoning.
> >
> > It would just be a call to create a platform device instead. How is that a
> mess?
> >
> > It's describing firmware. We have DT for describing h/w we've failed
> > to make discoverable. We should not repeat that and just describe
> firmware in DT.
> > Make the firmware discoverable! Though there are cases like firmware
> > provided clocks where we still need something in DT, but this is not
> > one of them.
> >
> > > >
> > > > For my part I referred to
> > > > watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT, &pdev-
> > > > >dev); in the driver, which guarantees that the timeout property
> > > > >will not be
> > > > used to set the timeout. A more common implementation would have
> > > > been
> > > >
> > > > imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
> > > > ret = watchdog_init_timeout(imx_sc_wdd, timeout,
> > > > &pdev->dev);
> > > >
> > > > where 'timeout' is the module parameter. Which is actually not
> > > > used in your driver.
> > > > Hmm ... I wasn't careful enough with my review. The timeout
> > > > initialization as- is doesn't make sense. I'll comment on that in the patch.
> > >
> > > I understand now, in our cases, I would still prefer to have
> > > watchdog node under the SCU parent node, since there could be other
> > > property setting difference between different i.MX platforms with
> > > system controller watchdog inside, using the SCU node to instantiate
> > > makes us a little confused about the watchdog, so if it is NOT that
> > > critical, I think we should keep watchdog node. But to make the
> > > watchdog driver more generic for other i.MX platforms, I changed the
> > > compatible string to
> > "fsl,imx-sc-wdt" in driver, and each SoC should has it as fallback if
> > it can reuse this watchdog driver.
> >
> > You handle differences between SoCs by having specific compatibles. So
> > "fsl,imx-sc-wdt" moves in the wrong direction assuming we have a node
> > in the first place.
> >
> > Rob
On Wed, Mar 13, 2019 at 3:11 AM Anson Huang <[email protected]> wrote:
>
> Hi, Rob
> Do you have any feedback about adding imx scu watchdog node in dts? Thanks.
Yes, in my reply 2 days ago.
Rob
Hi, Rob
Best Regards!
Anson Huang
> -----Original Message-----
> From: Rob Herring [mailto:[email protected]]
> Sent: 2019年3月12日 5:26
> To: Aisheng Dong <[email protected]>
> Cc: Anson Huang <[email protected]>; Guenter Roeck <linux@roeck-
> us.net>; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Andy Gross <[email protected]>; dl-
> linux-imx <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Daniel Baluta
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; Jens Wiklander <[email protected]>
> Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog
> binding
>
> +Jens W
>
> On Thu, Mar 7, 2019 at 6:22 AM Aisheng Dong <[email protected]>
> wrote:
> >
> > Hi Rob,
> >
> > > > > I think Rob suggested that the SCU parent driver should
> > > > > instantiate the watchdog without explicit watchdog node. That
> > > > > would be possible, but it currently uses
> > > > > devm_of_platform_populate() to do the instantiation, and
> > > > > changing that would be a mess. Besides, it does sem to me that
> > > > > your suggested node would describe the hardware, so I am not
> > > > > sure I understand the
> > > reasoning.
> > >
> > > It would just be a call to create a platform device instead. How is that a
> mess?
> > >
> > > It's describing firmware. We have DT for describing h/w we've failed
> > > to make discoverable. We should not repeat that and just describe
> firmware in DT.
> > > Make the firmware discoverable! Though there are cases like firmware
> > > provided clocks where we still need something in DT, but this is not
> > > one of them.
> > >
> >
> > The watchdog node here in question actually is not using SCU firmware call.
> > Due to security requirement by SCU, watchdog can only be accessed in
> > security mode, for IMX case, via ARM Trust Firmware. That means the
> > watchdog used in Linux actually is using ARM SMC call and does not
> > depend SCU driver. So It would be strange for SCU driver to instantiate it.
> >
> > For this situation, do you think we can move watchdog out of scu node?
> > Maybe rename the compatible string like "fsl,imx8qxp-sip-watchdog"
> > because it's actually a watchdog serviced by ATF firmware.
>
> Yes, but that creates more questions. What exactly does ATF talk to for the
> watchdog? The SCU firmware?
Yes, ATF talks to SCU firmware directly, Linux kernel watchdog driver call SMC instructions
to send command to ATF, and ATF will call SCU firmware API to finish the operation requested
by Linux kernel watchdog driver.
>
> Maybe ATF should define and provide a standard watchdog interface? It is
> still a question of making the firmware discoverable rather than trying to
> describe the firmware in DT.
The SMC call by Linux kernel watchdog already follow the SIP(silicon provider) standard, each
SoC can define its own protocol for SIP. ATF does NOT have a standard common watchdog interface
now, since it is more like a platform specific feature, most of platforms can control watchdog directly
from Linux kernel I think.
So, do you have suggestion for this case? Either find a place in DT to put watchdog node, or make it
a build-in device inside SCU driver? Or you have other better ideas?
Thanks,
Anson.
>
> Rob