On 10/12/19 3:05 AM, Xingyu Chen wrote:

Did you try COMPILE_TEST (eg allmodconfig) on, say x86_64 ?
AFAICS the meson sm calls are only available if MESON_SM is
enabled, and that depends on both ARCH_MESON and ARM64_4K_PAGES.
This dependency is not expressed here, and neither is enabled
with COMPILE_TEST.

This is equivalent to
	return ret;
or even
	return meson_sm_call(...);

+}
+
+static int meson_sec_wdt_stop(struct watchdog_device *wdt_dev)
+{
+	int ret;
+	struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+	ret = meson_sm_call(data->fw, SM_WATCHDOG_OPS, NULL,
+			    MESON_SIP_WDT_DISABLE, 0, 0, 0, 0);
+	if (ret)
+		return ret;
+
+	return 0;

Same as above.

+}
+
+static int meson_sec_wdt_ping(struct watchdog_device *wdt_dev)
+{
+	struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+	meson_sm_call(data->fw, SM_WATCHDOG_OPS, NULL,
+		      MESON_SIP_WDT_PING, 0, 0, 0, 0);
+
+	return 0;

Why ignore errors ?

+}
+
+static int meson_sec_wdt_set_timeout(struct watchdog_device *wdt_dev,
+				     unsigned int timeout)
+{
+	int ret;
+	struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+	wdt_dev->timeout = timeout;
+	meson_sec_wdt_ping(wdt_dev);
+

Unconditionally ? Also, the core does that after setting the timeout.

+	ret = meson_sm_call(data->fw, SM_WATCHDOG_OPS, NULL,
+			    MESON_SIP_WDT_SETTIMEOUT,
+			    wdt_dev->timeout, 0, 0, 0);
+	if (ret)
+		return ret;
+
+	return 0;

same as above.

+}
+
+static unsigned int meson_sec_wdt_get_timeleft(struct watchdog_device *wdt_dev)
+{
+	int ret;
+	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 0;

Really ? Why ? 0 is most definitely incorrect here.

+
+	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))
+		meson_sec_wdt_start(&data->wdt_dev);

No error return ?

+
+	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))
+		meson_sec_wdt_stop(&data->wdt_dev);

No error return ?

+
+	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 -ENODEV;

ENODEV is wrong here.

+	}
+
+	data->fw = meson_sm_get(sm_np);
+	of_node_put(sm_np);
+	if (!data->fw)
+		return -EPROBE_DEFER;

How do you know ?

+
+	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);
+

No watchdog_init_timeout() ? Any special reason for not supporting
to set the timeout with a devicetree property ?

+	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 [off-list ref]");
+MODULE_DESCRIPTION("Amlogic Secure Watchdog Timer Driver");
+MODULE_LICENSE("Dual BSD/GPL");

BSD or MIT ? This should match the license identifier.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel