Hi, Guenter On 2019/10/21 0:56, Guenter Roeck wrote:
On 10/18/19 1:33 AM, Xingyu Chen wrote:The watchdog controller on the Meson-A/C series SoCs is moved to secure world, watchdog operation needs to be done in secure EL3 mode via ATF, Non-secure world can call SMC instruction to trap to AFT for watchdog operation. Signed-off-by: Xingyu Chen <xingyu.chen@xxxxxxxxxxx> --- drivers/watchdog/Kconfig | 17 ++++ drivers/watchdog/Makefile | 1 +drivers/watchdog/meson_sec_wdt.c | 187 +++++++++++++++++++++++++++++++++++++++3 files changed, 205 insertions(+) create mode 100644 drivers/watchdog/meson_sec_wdt.c diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 58e7c10..e84be42 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -826,6 +826,23 @@ config MESON_GXBB_WATCHDOG To compile this driver as a module, choose M here: the module will be called meson_gxbb_wdt. +config MESON_SEC_WATCHDOG + tristate "Amlogic Meson Secure watchdog support" + depends on MESON_SM + depends on ARCH_MESON || COMPILE_TESTThis dependency is pointless. MESON_SM already depends on ARCH_MESON, thus specifying "COMPILE_TEST" here adds no value but only creates confusion.
Thanks for your analysis, perhaps i should remove the line below. - depends on ARCH_MESON || COMPILE_TEST Is it ok to modify code above like this ?
+ select WATCHDOG_CORE + help + The watchdog controller on the Meson-A/C series SoCs is moved to + secure world, watchdog operation needs to be done in secure EL3 + mode via ATF, non-secure world can call SMC instruction to trap + to ATF for the watchdog operation. + + Say Y here if watchdog controller on Meson SoCs is located in + secure world. + + To compile this driver as a module, choose M here: the + module will be called meson_sec_wdt. + config MESON_WATCHDOG tristate "Amlogic Meson SoCs watchdog support" depends on ARCH_MESON || COMPILE_TEST diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 2ee352b..5e6b73d 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -78,6 +78,7 @@ obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o obj-$(CONFIG_MESON_GXBB_WATCHDOG) += meson_gxbb_wdt.o +obj-$(CONFIG_MESON_SEC_WATCHDOG) += meson_sec_wdt.o obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.odiff --git a/drivers/watchdog/meson_sec_wdt.c b/drivers/watchdog/meson_sec_wdt.cnew file mode 100644 index 00000000..86bd87c --- /dev/null +++ b/drivers/watchdog/meson_sec_wdt.c @@ -0,0 +1,187 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Copyright (c) 2019 Amlogic, Inc. All rights reserved. + * Author: Xingyu Chen <xingyu.chen@xxxxxxxxxxx> + * + */ +#include <linux/err.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/types.h> +#include <linux/watchdog.h> +#include <linux/firmware/meson/meson_sm.h> + +#define MESON_SIP_WDT_DISABLE 0x1 +#define MESON_SIP_WDT_ENABLE 0x2 +#define MESON_SIP_WDT_PING 0x3 +#define MESON_SIP_WDT_INIT 0x4 +#define MESON_SIP_WDT_RESETNOW 0x5 +#define MESON_SIP_WDT_SETTIMEOUT 0x6 +#define MESON_SIP_WDT_GETTIMELEFT 0x7 + +#define DEFAULT_TIMEOUT 30 /* seconds */ + +/* + * Watchdog timer tick is set to 1ms in secfw side, and tick count is + * stored in the bit[16-31] of WATCHDOG_CNT register, so the maximum + * timeout value is 0xffff ms. + */ +#define MAX_TIMEOUT_MS 0xFFFF + +struct meson_sec_wdt { + struct watchdog_device wdt_dev; + struct meson_sm_firmware *fw; +}; + +static int meson_sec_wdt_start(struct watchdog_device *wdt_dev) +{ + struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev); + + return meson_sm_call(data->fw, SM_WATCHDOG_OPS, NULL, + MESON_SIP_WDT_ENABLE, 0, 0, 0, 0); +} + +static int meson_sec_wdt_stop(struct watchdog_device *wdt_dev) +{ + struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev); + + return meson_sm_call(data->fw, SM_WATCHDOG_OPS, NULL, + MESON_SIP_WDT_DISABLE, 0, 0, 0, 0); +} + +static int meson_sec_wdt_ping(struct watchdog_device *wdt_dev) +{ + struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev); + + return meson_sm_call(data->fw, SM_WATCHDOG_OPS, NULL, + MESON_SIP_WDT_PING, 0, 0, 0, 0); +} + +static int meson_sec_wdt_set_timeout(struct watchdog_device *wdt_dev, + unsigned int timeout) +{ + struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev); + + wdt_dev->timeout = timeout; + + return meson_sm_call(data->fw, SM_WATCHDOG_OPS, NULL, + MESON_SIP_WDT_SETTIMEOUT, + wdt_dev->timeout, 0, 0, 0); +} ++static unsigned int meson_sec_wdt_get_timeleft(struct watchdog_device *wdt_dev)+{ + int ret; + unsigned int timeleft; + struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev); + + ret = meson_sm_call(data->fw, SM_WATCHDOG_OPS, &timeleft, + MESON_SIP_WDT_GETTIMELEFT, 0, 0, 0, 0); + + if (ret) + return ret;Meh, that doesn't work. I just realized that the return type is unsigned, so returning a negative error code is pointless. Guess we'll have to live with returning 0 in this case after all. I wonder if we should fix the API and return an integer (with negative error code), but that is a different question.
Thanks for your review.IMO, if returning an integer, and the value which copy to user buf should be formatted with %d instead of %u (see timeleft_show), it will cause the max value of timeleft is reduced from 4294967295 to 2147483647. but i'am not sure whether it will bring risk.
So i also think returning 0 may be better in this case.
Sorry for the confusion - I should have noticed that before.+ + return timeleft; +} + +static const struct watchdog_ops meson_sec_wdt_ops = { + .start = meson_sec_wdt_start, + .stop = meson_sec_wdt_stop, + .ping = meson_sec_wdt_ping, + .set_timeout = meson_sec_wdt_set_timeout, + .get_timeleft = meson_sec_wdt_get_timeleft, +}; + +static const struct watchdog_info meson_sec_wdt_info = { + .identity = "Meson Secure Watchdog Timer",+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,+}; + +static int __maybe_unused meson_sec_wdt_resume(struct device *dev) +{ + struct meson_sec_wdt *data = dev_get_drvdata(dev); + + if (watchdog_active(&data->wdt_dev)) + return meson_sec_wdt_start(&data->wdt_dev); + + return 0; +} + +static int __maybe_unused meson_sec_wdt_suspend(struct device *dev) +{ + struct meson_sec_wdt *data = dev_get_drvdata(dev); + + if (watchdog_active(&data->wdt_dev)) + return meson_sec_wdt_stop(&data->wdt_dev); + + return 0; +} + +static const struct dev_pm_ops meson_sec_wdt_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(meson_sec_wdt_suspend, meson_sec_wdt_resume) +}; + +static const struct of_device_id meson_sec_wdt_dt_ids[] = { + { .compatible = "amlogic,meson-sec-wdt", }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, meson_sec_wdt_dt_ids); + +static int meson_sec_wdt_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct meson_sec_wdt *data; + struct device_node *sm_np; + int ret; + + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + sm_np = of_parse_phandle(pdev->dev.of_node, "secure-monitor", 0); + if (!sm_np) { + dev_err(&pdev->dev, "no secure-monitor node\n"); + return -EINVAL; + } + + data->fw = meson_sm_get(sm_np); + of_node_put(sm_np); + if (!data->fw) + return -EPROBE_DEFER; + + platform_set_drvdata(pdev, data); + + data->wdt_dev.parent = dev; + data->wdt_dev.info = &meson_sec_wdt_info; + data->wdt_dev.ops = &meson_sec_wdt_ops; + data->wdt_dev.max_hw_heartbeat_ms = MAX_TIMEOUT_MS; + data->wdt_dev.min_timeout = 1; + data->wdt_dev.timeout = DEFAULT_TIMEOUT; + watchdog_set_drvdata(&data->wdt_dev, data); + + ret = meson_sm_call(data->fw, SM_WATCHDOG_OPS, NULL, + MESON_SIP_WDT_INIT, + data->wdt_dev.timeout, 0, 0, 0); + if (ret) + return ret; + + watchdog_stop_on_reboot(&data->wdt_dev); + + return devm_watchdog_register_device(dev, &data->wdt_dev); +} + +static struct platform_driver meson_sec_wdt_driver = { + .probe = meson_sec_wdt_probe, + .driver = { + .name = "meson-sec-wdt", + .pm = &meson_sec_wdt_pm_ops, + .of_match_table = meson_sec_wdt_dt_ids, + }, +}; + +module_platform_driver(meson_sec_wdt_driver); + +MODULE_AUTHOR("Xingyu Chen <xingyu.chen@xxxxxxxxxxx>"); +MODULE_DESCRIPTION("Amlogic Secure Watchdog Timer Driver"); +MODULE_LICENSE("Dual MIT/GPL");.
