linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller watchdog support
@ 2019-02-28  7:59 Anson Huang
  2019-02-28  7:59 ` [PATCH V4 2/2] arm64: defconfig: add support for i.MX system controller watchdog Anson Huang
  2019-02-28 14:58 ` [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller watchdog support Guenter Roeck
  0 siblings, 2 replies; 6+ messages in thread
From: Anson Huang @ 2019-02-28  7:59 UTC (permalink / raw)
  To: catalin.marinas, will.deacon, wim, linux, shawnguo, s.hauer,
	kernel, festevam, Andy Gross, heiko, horms+renesas, arnd, olof,
	jagan, bjorn.andersson, enric.balletbo, marc.w.gonzalez,
	linux-arm-kernel, linux-kernel, linux-watchdog
  Cc: dl-linux-imx

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 <Anson.Huang@nxp.com>
---
Changes since V3:
        - add ARM64 dependency for this driver;
        - change SPDX license to GPL-2.0 to match module license;
        - register platform device in driver instead of getting from dts;
	- return linux error code instead of system controller firmware error code for watchdog operation
	  failed case.
---
 drivers/watchdog/Kconfig      |  13 +++
 drivers/watchdog/Makefile     |   1 +
 drivers/watchdog/imx_sc_wdt.c | 201 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 215 insertions(+)
 create mode 100644 drivers/watchdog/imx_sc_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 65c3c42..8c6575e 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 && ARM64) || 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..50b49b2
--- /dev/null
+++ b/drivers/watchdog/imx_sc_wdt.c
@@ -0,0 +1,201 @@
+// 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 struct platform_device *imx_sc_wdt_pdev;
+
+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 0;
+}
+
+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 -EACCES;
+
+	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 ? -EACCES : 0;
+}
+
+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 ? -EACCES : 0;
+}
+
+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 ? -EACCES : 0;
+}
+
+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, timeout, &pdev->dev);
+	if (ret)
+		dev_warn(&pdev->dev, "Failed to set timeout value, using default\n");
+
+	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 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",
+		.pm	= &imx_sc_wdt_pm_ops,
+	},
+};
+
+static int __init imx_sc_wdt_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&imx_sc_wdt_driver);
+	if (ret)
+		return ret;
+
+	imx_sc_wdt_pdev = platform_device_register_simple("imx-sc-wdt", -1, NULL, 0);
+	if (IS_ERR(imx_sc_wdt_pdev)) {
+		platform_driver_unregister(&imx_sc_wdt_driver);
+		return PTR_ERR(imx_sc_wdt_pdev);
+	}
+
+	return 0;
+}
+module_init(imx_sc_wdt_init);
+
+static void __exit imx_sc_wdt_exit(void)
+{
+	platform_driver_unregister(&imx_sc_wdt_driver);
+	platform_device_unregister(imx_sc_wdt_pdev);
+}
+module_exit(imx_sc_wdt_exit);
+
+MODULE_AUTHOR("Robin Gong <yibin.gong@nxp.com>");
+MODULE_DESCRIPTION("NXP i.MX system controller watchdog driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH V4 2/2] arm64: defconfig: add support for i.MX system controller watchdog
  2019-02-28  7:59 [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller watchdog support Anson Huang
@ 2019-02-28  7:59 ` Anson Huang
  2019-02-28 14:58 ` [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller watchdog support Guenter Roeck
  1 sibling, 0 replies; 6+ messages in thread
From: Anson Huang @ 2019-02-28  7:59 UTC (permalink / raw)
  To: catalin.marinas, will.deacon, wim, linux, shawnguo, s.hauer,
	kernel, festevam, Andy Gross, heiko, horms+renesas, arnd, olof,
	jagan, bjorn.andersson, enric.balletbo, marc.w.gonzalez,
	linux-arm-kernel, linux-kernel, linux-watchdog
  Cc: dl-linux-imx

Enable CONFIG_IMX_SC_WDT as module to support i.MX system
controller watchdog.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
no changes.
---
 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller watchdog support
  2019-02-28  7:59 [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller watchdog support Anson Huang
  2019-02-28  7:59 ` [PATCH V4 2/2] arm64: defconfig: add support for i.MX system controller watchdog Anson Huang
@ 2019-02-28 14:58 ` Guenter Roeck
  2019-03-01  3:46   ` Anson Huang
  1 sibling, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2019-02-28 14:58 UTC (permalink / raw)
  To: Anson Huang, catalin.marinas, will.deacon, wim, shawnguo,
	s.hauer, kernel, festevam, Andy Gross, heiko, horms+renesas,
	arnd, olof, jagan, bjorn.andersson, enric.balletbo,
	marc.w.gonzalez, linux-arm-kernel, linux-kernel, linux-watchdog
  Cc: dl-linux-imx

On 2/27/19 11:59 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 <Anson.Huang@nxp.com>
> ---
> Changes since V3:
>          - add ARM64 dependency for this driver;
>          - change SPDX license to GPL-2.0 to match module license;
>          - register platform device in driver instead of getting from dts;
> 	- return linux error code instead of system controller firmware error code for watchdog operation
> 	  failed case.
> ---
>   drivers/watchdog/Kconfig      |  13 +++
>   drivers/watchdog/Makefile     |   1 +
>   drivers/watchdog/imx_sc_wdt.c | 201 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 215 insertions(+)
>   create mode 100644 drivers/watchdog/imx_sc_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 65c3c42..8c6575e 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 && ARM64) || 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.
> +

With this patch applied, alpha:allmodconfig and almost all other
allmodconfig/allyesconfig builds fail with:

ERROR: "__arm_smccc_smc" [drivers/watchdog/imx_sc_wdt.ko] undefined!
scripts/Makefile.modpost:92: recipe for target '__modpost' failed
make[1]: *** [__modpost] Error 1
Makefile:1260: recipe for target 'modules' failed

as I told you before would happen. For the future, please consider
that forcing me to "prove" such failures does take a significant
amount of time, which is not always readily available.

Guenter


>   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..50b49b2
> --- /dev/null
> +++ b/drivers/watchdog/imx_sc_wdt.c
> @@ -0,0 +1,201 @@
> +// 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 struct platform_device *imx_sc_wdt_pdev;
> +
> +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 0;
> +}
> +
> +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 -EACCES;
> +
> +	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 ? -EACCES : 0;
> +}
> +
> +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 ? -EACCES : 0;
> +}
> +
> +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 ? -EACCES : 0;
> +}
> +
> +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, timeout, &pdev->dev);
> +	if (ret)
> +		dev_warn(&pdev->dev, "Failed to set timeout value, using default\n");
> +
> +	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 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",
> +		.pm	= &imx_sc_wdt_pm_ops,
> +	},
> +};
> +
> +static int __init imx_sc_wdt_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&imx_sc_wdt_driver);
> +	if (ret)
> +		return ret;
> +
> +	imx_sc_wdt_pdev = platform_device_register_simple("imx-sc-wdt", -1, NULL, 0);
> +	if (IS_ERR(imx_sc_wdt_pdev)) {
> +		platform_driver_unregister(&imx_sc_wdt_driver);
> +		return PTR_ERR(imx_sc_wdt_pdev);
> +	}
> +
> +	return 0;
> +}
> +module_init(imx_sc_wdt_init);
> +
> +static void __exit imx_sc_wdt_exit(void)
> +{
> +	platform_driver_unregister(&imx_sc_wdt_driver);
> +	platform_device_unregister(imx_sc_wdt_pdev);
> +}
> +module_exit(imx_sc_wdt_exit);
> +
> +MODULE_AUTHOR("Robin Gong <yibin.gong@nxp.com>");
> +MODULE_DESCRIPTION("NXP i.MX system controller watchdog driver");
> +MODULE_LICENSE("GPL v2");
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller watchdog support
  2019-02-28 14:58 ` [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller watchdog support Guenter Roeck
@ 2019-03-01  3:46   ` Anson Huang
  2019-03-01  4:21     ` Guenter Roeck
  0 siblings, 1 reply; 6+ messages in thread
From: Anson Huang @ 2019-03-01  3:46 UTC (permalink / raw)
  To: Guenter Roeck, catalin.marinas, will.deacon, wim, shawnguo,
	s.hauer, kernel, festevam, Andy Gross, heiko, horms+renesas,
	arnd, olof, jagan, bjorn.andersson, enric.balletbo,
	marc.w.gonzalez, linux-arm-kernel, linux-kernel, linux-watchdog
  Cc: dl-linux-imx

Hi, Guenter

Best Regards!
Anson Huang

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: 2019年2月28日 22:58
> To: Anson Huang <anson.huang@nxp.com>; catalin.marinas@arm.com;
> will.deacon@arm.com; wim@linux-watchdog.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> Andy Gross <andy.gross@linaro.org>; heiko@sntech.de;
> horms+renesas@verge.net.au; arnd@arndb.de; olof@lixom.net;
> jagan@amarulasolutions.com; bjorn.andersson@linaro.org;
> enric.balletbo@collabora.com; marc.w.gonzalez@free.fr; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> watchdog@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller
> watchdog support
> 
> On 2/27/19 11:59 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 <Anson.Huang@nxp.com>
> > ---
> > Changes since V3:
> >          - add ARM64 dependency for this driver;
> >          - change SPDX license to GPL-2.0 to match module license;
> >          - register platform device in driver instead of getting from dts;
> > 	- return linux error code instead of system controller firmware error
> code for watchdog operation
> > 	  failed case.
> > ---
> >   drivers/watchdog/Kconfig      |  13 +++
> >   drivers/watchdog/Makefile     |   1 +
> >   drivers/watchdog/imx_sc_wdt.c | 201
> ++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 215 insertions(+)
> >   create mode 100644 drivers/watchdog/imx_sc_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > 65c3c42..8c6575e 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 && ARM64) || 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.
> > +
> 
> With this patch applied, alpha:allmodconfig and almost all other
> allmodconfig/allyesconfig builds fail with:
> 
> ERROR: "__arm_smccc_smc" [drivers/watchdog/imx_sc_wdt.ko] undefined!
> scripts/Makefile.modpost:92: recipe for target '__modpost' failed
> make[1]: *** [__modpost] Error 1
> Makefile:1260: recipe for target 'modules' failed
> 
> as I told you before would happen. For the future, please consider that
> forcing me to "prove" such failures does take a significant amount of time,
> which is not always readily available.

Sorry for wasting your time, it is just because I misunderstand your point, NOT
that I did NOT force you to prove it.

I am a little confuse, since this is new to me about these configs, I looked into
other drivers which also use arm_smccc_smc, they do NOT add special config
dependency for the failure build cause as you said, can you be more detail about
this build issue, I tried below build, but the failure I met is other, so could you please
advise how to fix such dependency issue, adding dummy function looks like NOT
a good way since this arm_smccc_smc() API is widely used in kernel, there should be
some common solution for it, Thanks in advanced and appreciate for your time.

Anson.

anson@anson-OptiPlex-790:~/workspace/stash/linux-next$ make.cross ARCH=arm64 allyesconfig
anson@anson-OptiPlex-790:~/workspace/stash/linux-next$ make.cross ARCH=arm64

arch/arm64/kernel/traps.o: In function `early_brk64':
traps.c:(.init.text+0x24): relocation truncated to fit: R_AARCH64_CALL26 against `.text'
arch/arm64/kernel/entry.o:(.altinstr_replacement+0x70): relocation truncated to fit: R_AARCH64_JUMP26 against `.entry.text'
arch/arm64/kernel/entry.o:(.altinstr_replacement+0x84): relocation truncated to fit: R_AARCH64_JUMP26 against `.entry.text'
arch/arm64/kernel/entry.o:(.altinstr_replacement+0x98): relocation truncated to fit: R_AARCH64_JUMP26 against `.entry.text'


> 
> Guenter
> 
> 
> >   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..50b49b2
> > --- /dev/null
> > +++ b/drivers/watchdog/imx_sc_wdt.c
> > @@ -0,0 +1,201 @@
> > +// 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 struct platform_device *imx_sc_wdt_pdev;
> > +
> > +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 0;
> > +}
> > +
> > +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 -EACCES;
> > +
> > +	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 ? -EACCES : 0;
> > +}
> > +
> > +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 ? -EACCES : 0;
> > +}
> > +
> > +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 ? -EACCES : 0;
> > +}
> > +
> > +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, timeout, &pdev->dev);
> > +	if (ret)
> > +		dev_warn(&pdev->dev, "Failed to set timeout value, using
> > +default\n");
> > +
> > +	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 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",
> > +		.pm	= &imx_sc_wdt_pm_ops,
> > +	},
> > +};
> > +
> > +static int __init imx_sc_wdt_init(void) {
> > +	int ret;
> > +
> > +	ret = platform_driver_register(&imx_sc_wdt_driver);
> > +	if (ret)
> > +		return ret;
> > +
> > +	imx_sc_wdt_pdev = platform_device_register_simple("imx-sc-wdt", -
> 1, NULL, 0);
> > +	if (IS_ERR(imx_sc_wdt_pdev)) {
> > +		platform_driver_unregister(&imx_sc_wdt_driver);
> > +		return PTR_ERR(imx_sc_wdt_pdev);
> > +	}
> > +
> > +	return 0;
> > +}
> > +module_init(imx_sc_wdt_init);
> > +
> > +static void __exit imx_sc_wdt_exit(void) {
> > +	platform_driver_unregister(&imx_sc_wdt_driver);
> > +	platform_device_unregister(imx_sc_wdt_pdev);
> > +}
> > +module_exit(imx_sc_wdt_exit);
> > +
> > +MODULE_AUTHOR("Robin Gong <yibin.gong@nxp.com>");
> > +MODULE_DESCRIPTION("NXP i.MX system controller watchdog driver");
> > +MODULE_LICENSE("GPL v2");
> >


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller watchdog support
  2019-03-01  3:46   ` Anson Huang
@ 2019-03-01  4:21     ` Guenter Roeck
  2019-03-01  4:51       ` Anson Huang
  0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2019-03-01  4:21 UTC (permalink / raw)
  To: Anson Huang, catalin.marinas, will.deacon, wim, shawnguo,
	s.hauer, kernel, festevam, Andy Gross, heiko, horms+renesas,
	arnd, olof, jagan, bjorn.andersson, enric.balletbo,
	marc.w.gonzalez, linux-arm-kernel, linux-kernel, linux-watchdog
  Cc: dl-linux-imx

On 2/28/19 7:46 PM, Anson Huang wrote:
> Hi, Guenter
> 
> Best Regards!
> Anson Huang
> 
>> -----Original Message-----
>> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
>> Roeck
>> Sent: 2019年2月28日 22:58
>> To: Anson Huang <anson.huang@nxp.com>; catalin.marinas@arm.com;
>> will.deacon@arm.com; wim@linux-watchdog.org; shawnguo@kernel.org;
>> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
>> Andy Gross <andy.gross@linaro.org>; heiko@sntech.de;
>> horms+renesas@verge.net.au; arnd@arndb.de; olof@lixom.net;
>> jagan@amarulasolutions.com; bjorn.andersson@linaro.org;
>> enric.balletbo@collabora.com; marc.w.gonzalez@free.fr; linux-arm-
>> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
>> watchdog@vger.kernel.org
>> Cc: dl-linux-imx <linux-imx@nxp.com>
>> Subject: Re: [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller
>> watchdog support
>>
>> On 2/27/19 11:59 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 <Anson.Huang@nxp.com>
>>> ---
>>> Changes since V3:
>>>           - add ARM64 dependency for this driver;
>>>           - change SPDX license to GPL-2.0 to match module license;
>>>           - register platform device in driver instead of getting from dts;
>>> 	- return linux error code instead of system controller firmware error
>> code for watchdog operation
>>> 	  failed case.
>>> ---
>>>    drivers/watchdog/Kconfig      |  13 +++
>>>    drivers/watchdog/Makefile     |   1 +
>>>    drivers/watchdog/imx_sc_wdt.c | 201
>> ++++++++++++++++++++++++++++++++++++++++++
>>>    3 files changed, 215 insertions(+)
>>>    create mode 100644 drivers/watchdog/imx_sc_wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
>>> 65c3c42..8c6575e 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 && ARM64) || 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.
>>> +
>>
>> With this patch applied, alpha:allmodconfig and almost all other
>> allmodconfig/allyesconfig builds fail with:
>>
>> ERROR: "__arm_smccc_smc" [drivers/watchdog/imx_sc_wdt.ko] undefined!
>> scripts/Makefile.modpost:92: recipe for target '__modpost' failed
>> make[1]: *** [__modpost] Error 1
>> Makefile:1260: recipe for target 'modules' failed
>>
>> as I told you before would happen. For the future, please consider that
>> forcing me to "prove" such failures does take a significant amount of time,
>> which is not always readily available.
> 
> Sorry for wasting your time, it is just because I misunderstand your point, NOT
> that I did NOT force you to prove it.
> 
> I am a little confuse, since this is new to me about these configs, I looked into
> other drivers which also use arm_smccc_smc, they do NOT add special config
> dependency for the failure build cause as you said, can you be more detail about
> this build issue, I tried below build, but the failure I met is other, so could you please
> advise how to fix such dependency issue, adding dummy function looks like NOT
> a good way since this arm_smccc_smc() API is widely used in kernel, there should be
> some common solution for it, Thanks in advanced and appreciate for your time.
> 

I am quite sure that the other drivers calling arm_smccc_smc don't have
"|| COMPILE_TEST" as dependency, or they have a secondary dependency.

Randomly picking some

RTC_DRV_IMX_SC - no.
ARM_RK3399_DMC_DEVFREQ - no.
PHY_MVEBU_A3700_COMPHY - yes, but it also has "depends on HAVE_ARM_SMCCC" as
separate line, limiting its scope.

You can either drop "|| COMPILE_TEST" or add "depends on HAVE_ARM_SMCCC"
in a separate line. There may be other options, but I did not explore them.

Guenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller watchdog support
  2019-03-01  4:21     ` Guenter Roeck
@ 2019-03-01  4:51       ` Anson Huang
  0 siblings, 0 replies; 6+ messages in thread
From: Anson Huang @ 2019-03-01  4:51 UTC (permalink / raw)
  To: Guenter Roeck, catalin.marinas, will.deacon, wim, shawnguo,
	s.hauer, kernel, festevam, Andy Gross, heiko, horms+renesas,
	arnd, olof, jagan, bjorn.andersson, enric.balletbo,
	marc.w.gonzalez, linux-arm-kernel, linux-kernel, linux-watchdog
  Cc: dl-linux-imx

Hi, Guenter

Best Regards!
Anson Huang

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: 2019年3月1日 12:21
> To: Anson Huang <anson.huang@nxp.com>; catalin.marinas@arm.com;
> will.deacon@arm.com; wim@linux-watchdog.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> Andy Gross <andy.gross@linaro.org>; heiko@sntech.de;
> horms+renesas@verge.net.au; arnd@arndb.de; olof@lixom.net;
> jagan@amarulasolutions.com; bjorn.andersson@linaro.org;
> enric.balletbo@collabora.com; marc.w.gonzalez@free.fr; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> watchdog@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller
> watchdog support
> 
> On 2/28/19 7:46 PM, Anson Huang wrote:
> > Hi, Guenter
> >
> > Best Regards!
> > Anson Huang
> >
> >> -----Original Message-----
> >> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> >> Roeck
> >> Sent: 2019年2月28日 22:58
> >> To: Anson Huang <anson.huang@nxp.com>; catalin.marinas@arm.com;
> >> will.deacon@arm.com; wim@linux-watchdog.org; shawnguo@kernel.org;
> >> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> >> Andy Gross <andy.gross@linaro.org>; heiko@sntech.de;
> >> horms+renesas@verge.net.au; arnd@arndb.de; olof@lixom.net;
> >> jagan@amarulasolutions.com; bjorn.andersson@linaro.org;
> >> enric.balletbo@collabora.com; marc.w.gonzalez@free.fr; linux-arm-
> >> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> >> watchdog@vger.kernel.org
> >> Cc: dl-linux-imx <linux-imx@nxp.com>
> >> Subject: Re: [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system
> >> controller watchdog support
> >>
> >> On 2/27/19 11:59 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 <Anson.Huang@nxp.com>
> >>> ---
> >>> Changes since V3:
> >>>           - add ARM64 dependency for this driver;
> >>>           - change SPDX license to GPL-2.0 to match module license;
> >>>           - register platform device in driver instead of getting from dts;
> >>> 	- return linux error code instead of system controller firmware
> >>> error
> >> code for watchdog operation
> >>> 	  failed case.
> >>> ---
> >>>    drivers/watchdog/Kconfig      |  13 +++
> >>>    drivers/watchdog/Makefile     |   1 +
> >>>    drivers/watchdog/imx_sc_wdt.c | 201
> >> ++++++++++++++++++++++++++++++++++++++++++
> >>>    3 files changed, 215 insertions(+)
> >>>    create mode 100644 drivers/watchdog/imx_sc_wdt.c
> >>>
> >>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> >>> index 65c3c42..8c6575e 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 && ARM64) || 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.
> >>> +
> >>
> >> With this patch applied, alpha:allmodconfig and almost all other
> >> allmodconfig/allyesconfig builds fail with:
> >>
> >> ERROR: "__arm_smccc_smc" [drivers/watchdog/imx_sc_wdt.ko]
> undefined!
> >> scripts/Makefile.modpost:92: recipe for target '__modpost' failed
> >> make[1]: *** [__modpost] Error 1
> >> Makefile:1260: recipe for target 'modules' failed
> >>
> >> as I told you before would happen. For the future, please consider
> >> that forcing me to "prove" such failures does take a significant
> >> amount of time, which is not always readily available.
> >
> > Sorry for wasting your time, it is just because I misunderstand your
> > point, NOT that I did NOT force you to prove it.
> >
> > I am a little confuse, since this is new to me about these configs, I
> > looked into other drivers which also use arm_smccc_smc, they do NOT
> > add special config dependency for the failure build cause as you said,
> > can you be more detail about this build issue, I tried below build,
> > but the failure I met is other, so could you please advise how to fix
> > such dependency issue, adding dummy function looks like NOT a good way
> > since this arm_smccc_smc() API is widely used in kernel, there should be
> some common solution for it, Thanks in advanced and appreciate for your
> time.
> >
> 
> I am quite sure that the other drivers calling arm_smccc_smc don't have "||
> COMPILE_TEST" as dependency, or they have a secondary dependency.
> 
> Randomly picking some
> 
> RTC_DRV_IMX_SC - no.
> ARM_RK3399_DMC_DEVFREQ - no.
> PHY_MVEBU_A3700_COMPHY - yes, but it also has "depends on
> HAVE_ARM_SMCCC" as separate line, limiting its scope.
> 
> You can either drop "|| COMPILE_TEST" or add "depends on
> HAVE_ARM_SMCCC"
> in a separate line. There may be other options, but I did not explore them.

Many thanks for your detail info, I will add depends on " HAVE_ARM_SMCCC "
in V5 patch series.

Anson.

> 
> Guenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-03-01  4:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-28  7:59 [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller watchdog support Anson Huang
2019-02-28  7:59 ` [PATCH V4 2/2] arm64: defconfig: add support for i.MX system controller watchdog Anson Huang
2019-02-28 14:58 ` [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller watchdog support Guenter Roeck
2019-03-01  3:46   ` Anson Huang
2019-03-01  4:21     ` Guenter Roeck
2019-03-01  4:51       ` Anson Huang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).