The watchdog controller on the Meson-A/C series SoCs is moved to secure world,
We have to call SMC instruction to trap the ATF for watchdog operation. These
operations are different from previous SoCs, so we introduce a new watchdog
driver to support this kind of SoCs.
Changes since v1 at [0]:
- add a new dependency in Kconfig
- simplify/add the return operation
- remove useless ping operation when setting the timeout
- fix some return values
- fix the license statement
[0]:https://lore.kernel.org/linux-amlogic/[email protected]
Xingyu Chen (4):
firmware: meson_sm: add new SMC ID support for accessing secure
watchdog
dt-bindings: watchdog: add new binding for meson secure watchdog
watchdog: add meson secure watchdog driver
arm64: dts: a1: add secure watchdog controller
.../bindings/watchdog/amlogic,meson-sec-wdt.yaml | 34 ++++
arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 6 +
drivers/firmware/meson/meson_sm.c | 1 +
drivers/watchdog/Kconfig | 17 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/meson_sec_wdt.c | 187 +++++++++++++++++++++
include/linux/firmware/meson/meson_sm.h | 1 +
7 files changed, 247 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/amlogic,meson-sec-wdt.yaml
create mode 100644 drivers/watchdog/meson_sec_wdt.c
--
2.7.4
The new SMC ID is used to access secure registers by meson secure
watchdog driver.
Signed-off-by: Xingyu Chen <[email protected]>
---
drivers/firmware/meson/meson_sm.c | 1 +
include/linux/firmware/meson/meson_sm.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
index 1d5b4d7..46a44de 100644
--- a/drivers/firmware/meson/meson_sm.c
+++ b/drivers/firmware/meson/meson_sm.c
@@ -44,6 +44,7 @@ static const struct meson_sm_chip gxbb_chip = {
CMD(SM_EFUSE_WRITE, 0x82000031),
CMD(SM_EFUSE_USER_MAX, 0x82000033),
CMD(SM_GET_CHIP_ID, 0x82000044),
+ CMD(SM_WATCHDOG_OPS, 0x82000086),
{ /* sentinel */ },
},
};
diff --git a/include/linux/firmware/meson/meson_sm.h b/include/linux/firmware/meson/meson_sm.h
index 6669e2a..0934718 100644
--- a/include/linux/firmware/meson/meson_sm.h
+++ b/include/linux/firmware/meson/meson_sm.h
@@ -12,6 +12,7 @@ enum {
SM_EFUSE_WRITE,
SM_EFUSE_USER_MAX,
SM_GET_CHIP_ID,
+ SM_WATCHDOG_OPS,
};
struct meson_sm_firmware;
--
2.7.4
The binding targets the Meson-A/C series compatible SoCs, in which the
watchdog registers are in secure world.
Signed-off-by: Xingyu Chen <[email protected]>
---
.../bindings/watchdog/amlogic,meson-sec-wdt.yaml | 34 ++++++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/amlogic,meson-sec-wdt.yaml
diff --git a/Documentation/devicetree/bindings/watchdog/amlogic,meson-sec-wdt.yaml b/Documentation/devicetree/bindings/watchdog/amlogic,meson-sec-wdt.yaml
new file mode 100644
index 00000000..0bbc807
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/amlogic,meson-sec-wdt.yaml
@@ -0,0 +1,34 @@
+# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+# Copyright (c) 2019 Amlogic, Inc
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/power/amlogic,meson-sec-wdt.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Amlogic Meson Secure Watchdog Timer
+
+maintainers:
+ - Xingyu Chen <[email protected]>
+
+description: |+
+ Secure Watchdog Timer used in Meson-A/C series Compatible SoCs
+
+properties:
+ compatible:
+ enum:
+ - amlogic,meson-sec-wdt
+
+ secure-monitor:
+ description: phandle to the secure-monitor node
+ $ref: /schemas/types.yaml#/definitions/phandle
+
+required:
+ - compatible
+ - secure-monitor
+
+examples:
+ - |
+ watchdog {
+ compatible = "amlogic,meson-sec-wdt";
+ secure-monitor = <&sm>;
+ };
--
2.7.4
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 <[email protected]>
---
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_TEST
+ 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.o
diff --git a/drivers/watchdog/meson_sec_wdt.c b/drivers/watchdog/meson_sec_wdt.c
new 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 <[email protected]>
+ *
+ */
+#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;
+
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("Amlogic Secure Watchdog Timer Driver");
+MODULE_LICENSE("Dual MIT/GPL");
--
2.7.4
Enable secure watchdog controller for Meson-A1 SoC
Signed-off-by: Xingyu Chen <[email protected]>
---
arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
index 7210ad0..047c323 100644
--- a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
@@ -93,6 +93,12 @@
clock-names = "xtal", "pclk", "baud";
status = "disabled";
};
+
+ watchdog {
+ compatible = "amlogic,meson-sec-wdt";
+ secure-monitor = <&sm>;
+ status = "okay";
+ };
};
gic: interrupt-controller@ff901000 {
--
2.7.4
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 <[email protected]>
> ---
> 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_TEST
This dependency is pointless. MESON_SM already depends on ARCH_MESON,
thus specifying "COMPILE_TEST" here adds no value but only
creates confusion.
> + 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.o
> diff --git a/drivers/watchdog/meson_sec_wdt.c b/drivers/watchdog/meson_sec_wdt.c
> new 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 <[email protected]>
> + *
> + */
> +#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.
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 <[email protected]>");
> +MODULE_DESCRIPTION("Amlogic Secure Watchdog Timer Driver");
> +MODULE_LICENSE("Dual MIT/GPL");
>
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 <[email protected]>
>> ---
>> 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_TEST
>
> This 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.o
>> diff --git a/drivers/watchdog/meson_sec_wdt.c
>> b/drivers/watchdog/meson_sec_wdt.c
>> new 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 <[email protected]>
>> + *
>> + */
>> +#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 <[email protected]>");
>> +MODULE_DESCRIPTION("Amlogic Secure Watchdog Timer Driver");
>> +MODULE_LICENSE("Dual MIT/GPL");
>>
>
> .
>
On 10/21/19 1:03 AM, Xingyu Chen wrote:
> 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 <[email protected]>
>>> ---
>>> 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_TEST
>>
>> This 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 ?
Yes.
[ ... ]
>>> +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.
Not that it matters right now, but I don't think that limiting 'timeleft'
reporting to 2147483647 seconds, or ~68 years, would cause any risk.
It would just be a large patch changing several drivers all at once,
that is all.
>
> So i also think returning 0 may be better in this case.
Yes, please do that.
Thanks,
Guenter
Hi, Guenter
On 2019/10/21 21:38, Guenter Roeck wrote:
> On 10/21/19 1:03 AM, Xingyu Chen wrote:
>> 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 <[email protected]>
>>>> ---
>>>> 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_TEST
>>>
>>> This 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 ?
>
> Yes.
Thanks, fix it in next version.
> [ ... ]
>
>>>> +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, Thanks&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.
>
> Not that it matters right now, but I don't think that limiting 'timeleft'
> reporting to 2147483647 seconds, or ~68 years, would cause any risk.
> It would just be a large patch changing several drivers all at once,
> that is all.
>
>>
>> So i also think returning 0 may be better in this case.
>
> Yes, please do that.
Thanks, fix it in next version.
>
> Thanks,
> Guenter
>
> .
>