| Message ID | 20200421210403.v2.2.Ia92bb4d4ce84bcefeba1d00aaa1c1e919b6164ef@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add a watchdog driver that uses ARM Secure Monitor Calls. | expand |
On 4/21/20 4:05 AM, Evan Benn wrote: > From: Julius Werner <jwerner@chromium.org> > > This patch adds a watchdog driver that can be used on ARM systems > with the appropriate watchdog implemented in Secure Monitor firmware. > The driver communicates with firmware via a Secure Monitor Call. > This may be useful for platforms using TrustZone that want > the Secure Monitor firmware to have the final control over the watchdog. > > This is implemented on mt8173 chromebook devices oak, elm and hana in > arm trusted firmware file plat/mediatek/mt8173/drivers/wdt/wdt.c. > > Signed-off-by: Julius Werner <jwerner@chromium.org> > Signed-off-by: Evan Benn <evanbenn@chromium.org> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> You have made functional changes. Keeping a Reviewed-by: tag after functional changes is inappropriate unless the reviewer explicitly agreed to it. Guenter > > --- > > Changes in v4: > - Get smc-id from of property > - Return a1 instead of a0 in timeleft > > Changes in v3: > - Add optional get_timeleft op > - change name to arm_smc_wdt > > Changes in v2: > - use watchdog_stop_on_reboot > - use watchdog_stop_on_unregister > - use devm_watchdog_register_device > - remove smcwd_shutdown, smcwd_remove > - change error codes > > MAINTAINERS | 1 + > arch/arm64/configs/defconfig | 1 + > drivers/watchdog/Kconfig | 13 +++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/arm_smc_wdt.c | 194 +++++++++++++++++++++++++++++++++ > 5 files changed, 210 insertions(+) > create mode 100644 drivers/watchdog/arm_smc_wdt.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 0f2b39767bfa9..2b782bbff200a 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1462,6 +1462,7 @@ M: Julius Werner <jwerner@chromium.org> > R: Evan Benn <evanbenn@chromium.org> > S: Maintained > F: devicetree/bindings/watchdog/arm-smc-wdt.yaml > +F: drivers/watchdog/arm_smc_wdt.c > > ARM SMMU DRIVERS > M: Will Deacon <will@kernel.org> > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index 24e534d850454..0619df80f7575 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -513,6 +513,7 @@ CONFIG_UNIPHIER_THERMAL=y > CONFIG_WATCHDOG=y > CONFIG_ARM_SP805_WATCHDOG=y > CONFIG_ARM_SBSA_WATCHDOG=y > +CONFIG_ARM_SMC_WATCHDOG=y > CONFIG_S3C2410_WATCHDOG=y > CONFIG_DW_WATCHDOG=y > CONFIG_SUNXI_WATCHDOG=m > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 0663c604bd642..c440b576d23bf 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -867,6 +867,19 @@ config DIGICOLOR_WATCHDOG > To compile this driver as a module, choose M here: the > module will be called digicolor_wdt. > > +config ARM_SMC_WATCHDOG > + tristate "ARM Secure Monitor Call based watchdog support" > + depends on ARM || ARM64 > + depends on OF > + depends on HAVE_ARM_SMCCC > + select WATCHDOG_CORE > + help > + Say Y here to include support for a watchdog timer > + implemented by the EL3 Secure Monitor on ARM platforms. > + Requires firmware support. > + To compile this driver as a module, choose M here: the > + module will be called arm_smc_wdt. > + > config LPC18XX_WATCHDOG > tristate "LPC18xx/43xx Watchdog" > depends on ARCH_LPC18XX || COMPILE_TEST > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 6de2e4ceef190..97bed1d3d97cb 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -94,6 +94,7 @@ obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o > obj-$(CONFIG_RTD119X_WATCHDOG) += rtd119x_wdt.o > obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o > obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o > +obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o > > # X86 (i386 + ia64 + x86_64) Architecture > obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o > diff --git a/drivers/watchdog/arm_smc_wdt.c b/drivers/watchdog/arm_smc_wdt.c > new file mode 100644 > index 0000000000000..29d2573b2ca11 > --- /dev/null > +++ b/drivers/watchdog/arm_smc_wdt.c > @@ -0,0 +1,194 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * ARM Secure Monitor Call watchdog driver > + * > + * Copyright 2020 Google LLC. > + * Julius Werner <jwerner@chromium.org> > + * Based on mtk_wdt.c > + */ > + > +#include <linux/arm-smccc.h> > +#include <linux/err.h> > +#include <linux/module.h> > +#include <linux/moduleparam.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/types.h> > +#include <linux/watchdog.h> > +#include <uapi/linux/psci.h> > + > +#define DRV_NAME "arm_smc_wdt" > +#define DRV_VERSION "1.0" > + > +#define get_smc_func_id(wdd) (*(u32 *)watchdog_get_drvdata(wdd)) > +enum smcwd_call { > + SMCWD_INIT = 0, > + SMCWD_SET_TIMEOUT = 1, > + SMCWD_ENABLE = 2, > + SMCWD_PET = 3, > + SMCWD_GET_TIMELEFT = 4, > +}; > + > +static bool nowayout = WATCHDOG_NOWAYOUT; > +static unsigned int timeout; > + > +static int smcwd_call(unsigned long smc_func_id, enum smcwd_call call, > + unsigned long arg, struct arm_smccc_res *res) > +{ > + struct arm_smccc_res local_res; > + > + if (!res) > + res = &local_res; > + > + arm_smccc_smc(smc_func_id, call, arg, 0, 0, 0, 0, 0, res); > + > + if (res->a0 == PSCI_RET_NOT_SUPPORTED) > + return -ENODEV; > + if (res->a0 == PSCI_RET_INVALID_PARAMS) > + return -EINVAL; > + if (res->a0 != PSCI_RET_SUCCESS) > + return -EIO; > + return 0; > +} > + > +static int smcwd_ping(struct watchdog_device *wdd) > +{ > + return smcwd_call(get_smc_func_id(wdd), SMCWD_PET, 0, NULL); > +} > + > +static unsigned int smcwd_get_timeleft(struct watchdog_device *wdd) > +{ > + struct arm_smccc_res res; > + > + smcwd_call(get_smc_func_id(wdd), SMCWD_GET_TIMELEFT, 0, &res); > + return res.a1; > +} > + > +static int smcwd_set_timeout(struct watchdog_device *wdd, unsigned int timeout) > +{ > + int res; > + > + res = smcwd_call(get_smc_func_id(wdd), SMCWD_SET_TIMEOUT, timeout, > + NULL); > + if (!res) > + wdd->timeout = timeout; > + return res; > +} > + > +static int smcwd_stop(struct watchdog_device *wdd) > +{ > + return smcwd_call(get_smc_func_id(wdd), SMCWD_ENABLE, 0, NULL); > +} > + > +static int smcwd_start(struct watchdog_device *wdd) > +{ > + return smcwd_call(get_smc_func_id(wdd), SMCWD_ENABLE, 1, NULL); > +} > + > +static const struct watchdog_info smcwd_info = { > + .identity = DRV_NAME, > + .options = WDIOF_SETTIMEOUT | > + WDIOF_KEEPALIVEPING | > + WDIOF_MAGICCLOSE, > +}; > + > +static const struct watchdog_ops smcwd_ops = { > + .start = smcwd_start, > + .stop = smcwd_stop, > + .ping = smcwd_ping, > + .set_timeout = smcwd_set_timeout, > +}; > + > +static const struct watchdog_ops smcwd_timeleft_ops = { > + .start = smcwd_start, > + .stop = smcwd_stop, > + .ping = smcwd_ping, > + .set_timeout = smcwd_set_timeout, > + .get_timeleft = smcwd_get_timeleft, > +}; > + > +static int smcwd_probe(struct platform_device *pdev) > +{ > + struct watchdog_device *wdd; > + int err; > + struct arm_smccc_res res; > + u32 *smc_func_id; > + > + smc_func_id = > + devm_kzalloc(&pdev->dev, sizeof(*smc_func_id), GFP_KERNEL); > + if (!smc_func_id) > + return -ENOMEM; > + > + err = of_property_read_u32(pdev->dev.of_node, "arm,smc-id", > + smc_func_id); > + if (err < 0) > + return err; > + > + err = smcwd_call(*smc_func_id, SMCWD_INIT, 0, &res); > + if (err < 0) > + return err; > + > + wdd = devm_kzalloc(&pdev->dev, sizeof(*wdd), GFP_KERNEL); > + if (!wdd) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, wdd); > + watchdog_set_drvdata(wdd, smc_func_id); > + > + wdd->info = &smcwd_info; > + /* get_timeleft is optional */ > + if (smcwd_call(*smc_func_id, SMCWD_GET_TIMELEFT, 0, NULL)) > + wdd->ops = &smcwd_ops; > + else > + wdd->ops = &smcwd_timeleft_ops; > + wdd->timeout = res.a2; > + wdd->max_timeout = res.a2; > + wdd->min_timeout = res.a1; > + wdd->parent = &pdev->dev; > + > + watchdog_stop_on_reboot(wdd); > + watchdog_stop_on_unregister(wdd); > + watchdog_set_nowayout(wdd, nowayout); > + watchdog_init_timeout(wdd, timeout, &pdev->dev); > + err = smcwd_set_timeout(wdd, wdd->timeout); > + if (err) > + return err; > + > + err = devm_watchdog_register_device(&pdev->dev, wdd); > + if (err) > + return err; > + > + dev_info(&pdev->dev, > + "Watchdog registered (timeout=%d sec, nowayout=%d)\n", > + wdd->timeout, nowayout); > + > + return 0; > +} > + > +static const struct of_device_id smcwd_dt_ids[] = { > + { .compatible = "mediatek,mt8173-smc-wdt" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, smcwd_dt_ids); > + > +static struct platform_driver smcwd_driver = { > + .probe = smcwd_probe, > + .driver = { > + .name = DRV_NAME, > + .of_match_table = smcwd_dt_ids, > + }, > +}; > + > +module_platform_driver(smcwd_driver); > + > +module_param(timeout, uint, 0); > +MODULE_PARM_DESC(timeout, "Watchdog heartbeat in seconds"); > + > +module_param(nowayout, bool, 0); > +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Julius Werner <jwerner@chromium.org>"); > +MODULE_DESCRIPTION("ARM Secure Monitor Call Watchdog Driver"); > +MODULE_VERSION(DRV_VERSION); >
> +static int smcwd_call(unsigned long smc_func_id, enum smcwd_call call, > + unsigned long arg, struct arm_smccc_res *res) I think you should just take a struct watchdog_device* here and do the drvdata unpacking inside the function. > +static int smcwd_probe(struct platform_device *pdev) > +{ > + struct watchdog_device *wdd; > + int err; > + struct arm_smccc_res res; > + u32 *smc_func_id; > + > + smc_func_id = > + devm_kzalloc(&pdev->dev, sizeof(*smc_func_id), GFP_KERNEL); > + if (!smc_func_id) > + return -ENOMEM; nit: Could save the allocation by just casting the value itself to a pointer? Or is that considered too hacky? > +static const struct of_device_id smcwd_dt_ids[] = { > + { .compatible = "mediatek,mt8173-smc-wdt" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, smcwd_dt_ids); So I'm a bit confused about this... I thought the plan was to either use arm,smc-id and then there'll be no reason to put platform-specific quirks into the driver, so we can just use a generic "arm,smc-wdt" compatible string on all platforms; or we put individual compatible strings for each platform and use them to hardcode platform-specific differences (like the SMC ID) in the driver. But now you're kinda doing both by making the driver code platform-independent but still using a platform-specific compatible string, that doesn't seem to fit together. (If the driver can be platform independent, I think it's nicer to have a generic compatible string so that future platforms which support the same interface don't have to land code changes in order to just use the driver.)
On Wed, Apr 22, 2020 at 6:31 AM Julius Werner <jwerner@chromium.org> wrote: > > > +static int smcwd_call(unsigned long smc_func_id, enum smcwd_call call, > > + unsigned long arg, struct arm_smccc_res *res) > > I think you should just take a struct watchdog_device* here and do the > drvdata unpacking inside the function. That makes sense, I avoided it because smcwd_call's are made during 'probe', ~while we are 'constructing' the wdd. But this is C, so I think I have permission to do this! > > +static int smcwd_probe(struct platform_device *pdev) > > +{ > > + struct watchdog_device *wdd; > > + int err; > > + struct arm_smccc_res res; > > + u32 *smc_func_id; > > + > > + smc_func_id = > > + devm_kzalloc(&pdev->dev, sizeof(*smc_func_id), GFP_KERNEL); > > + if (!smc_func_id) > > + return -ENOMEM; > > nit: Could save the allocation by just casting the value itself to a > pointer? Or is that considered too hacky? I am not yet used to what hacks are allowed in the kernel. Where I learned C that would not be allowed. I assumed the kernel allocator has fast paths for tiny sizes though. > > +static const struct of_device_id smcwd_dt_ids[] = { > > + { .compatible = "mediatek,mt8173-smc-wdt" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, smcwd_dt_ids); > > So I'm a bit confused about this... I thought the plan was to either > use arm,smc-id and then there'll be no reason to put platform-specific > quirks into the driver, so we can just use a generic "arm,smc-wdt" > compatible string on all platforms; or we put individual compatible > strings for each platform and use them to hardcode platform-specific > differences (like the SMC ID) in the driver. But now you're kinda > doing both by making the driver code platform-independent but still > using a platform-specific compatible string, that doesn't seem to fit > together. (If the driver can be platform independent, I think it's > nicer to have a generic compatible string so that future platforms > which support the same interface don't have to land code changes in > order to just use the driver.) Yes I think you are correct. I got some reviews about the compatible name, but I think I misinterpreted those, and arm,smc-wdt would work. I did wonder if Xingyu from amlogic needed to modify the driver more, EG with different SMCWD_enum values for the amlogic chip, he could then just add an amlogic compatible and keep our devices running with the other compatible. Although of course it would be nicer if the amlogic firmware could copy the values here. Thanks Evan
On 4/21/20 1:31 PM, Julius Werner wrote: >> +static int smcwd_call(unsigned long smc_func_id, enum smcwd_call call, >> + unsigned long arg, struct arm_smccc_res *res) > > I think you should just take a struct watchdog_device* here and do the > drvdata unpacking inside the function. > >> +static int smcwd_probe(struct platform_device *pdev) >> +{ >> + struct watchdog_device *wdd; >> + int err; >> + struct arm_smccc_res res; >> + u32 *smc_func_id; >> + >> + smc_func_id = >> + devm_kzalloc(&pdev->dev, sizeof(*smc_func_id), GFP_KERNEL); >> + if (!smc_func_id) >> + return -ENOMEM; > > nit: Could save the allocation by just casting the value itself to a > pointer? Or is that considered too hacky? > Actually, the current code is what is hacky. I'd either do what you suggest, or allocate a structure such as struct local_data { u32 smc_func_id; struct watchdog_device wdd; }; and use it accordingly. Guenter >> +static const struct of_device_id smcwd_dt_ids[] = { >> + { .compatible = "mediatek,mt8173-smc-wdt" }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, smcwd_dt_ids); > > So I'm a bit confused about this... I thought the plan was to either > use arm,smc-id and then there'll be no reason to put platform-specific > quirks into the driver, so we can just use a generic "arm,smc-wdt" > compatible string on all platforms; or we put individual compatible > strings for each platform and use them to hardcode platform-specific > differences (like the SMC ID) in the driver. But now you're kinda > doing both by making the driver code platform-independent but still > using a platform-specific compatible string, that doesn't seem to fit > together. (If the driver can be platform independent, I think it's > nicer to have a generic compatible string so that future platforms > which support the same interface don't have to land code changes in > order to just use the driver.) >
Hi,Evan On 2020/4/21 19:05, Evan Benn wrote: > From: Julius Werner <jwerner@chromium.org> > > This patch adds a watchdog driver that can be used on ARM systems > with the appropriate watchdog implemented in Secure Monitor firmware. > The driver communicates with firmware via a Secure Monitor Call. > This may be useful for platforms using TrustZone that want > the Secure Monitor firmware to have the final control over the watchdog. > > This is implemented on mt8173 chromebook devices oak, elm and hana in > arm trusted firmware file plat/mediatek/mt8173/drivers/wdt/wdt.c. > > Signed-off-by: Julius Werner <jwerner@chromium.org> > Signed-off-by: Evan Benn <evanbenn@chromium.org> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > --- > > Changes in v4: > - Get smc-id from of property > - Return a1 instead of a0 in timeleft > > Changes in v3: > - Add optional get_timeleft op > - change name to arm_smc_wdt > > Changes in v2: > - use watchdog_stop_on_reboot > - use watchdog_stop_on_unregister > - use devm_watchdog_register_device > - remove smcwd_shutdown, smcwd_remove > - change error codes > > MAINTAINERS | 1 + > arch/arm64/configs/defconfig | 1 + > drivers/watchdog/Kconfig | 13 +++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/arm_smc_wdt.c | 194 +++++++++++++++++++++++++++++++++ > 5 files changed, 210 insertions(+) > create mode 100644 drivers/watchdog/arm_smc_wdt.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 0f2b39767bfa9..2b782bbff200a 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1462,6 +1462,7 @@ M: Julius Werner <jwerner@chromium.org> > R: Evan Benn <evanbenn@chromium.org> > S: Maintained > F: devicetree/bindings/watchdog/arm-smc-wdt.yaml > +F: drivers/watchdog/arm_smc_wdt.c > > ARM SMMU DRIVERS > M: Will Deacon <will@kernel.org> > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index 24e534d850454..0619df80f7575 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -513,6 +513,7 @@ CONFIG_UNIPHIER_THERMAL=y > CONFIG_WATCHDOG=y > CONFIG_ARM_SP805_WATCHDOG=y > CONFIG_ARM_SBSA_WATCHDOG=y > +CONFIG_ARM_SMC_WATCHDOG=y > CONFIG_S3C2410_WATCHDOG=y > CONFIG_DW_WATCHDOG=y > CONFIG_SUNXI_WATCHDOG=m > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 0663c604bd642..c440b576d23bf 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -867,6 +867,19 @@ config DIGICOLOR_WATCHDOG > To compile this driver as a module, choose M here: the > module will be called digicolor_wdt. > > +config ARM_SMC_WATCHDOG > + tristate "ARM Secure Monitor Call based watchdog support" > + depends on ARM || ARM64 > + depends on OF > + depends on HAVE_ARM_SMCCC > + select WATCHDOG_CORE > + help > + Say Y here to include support for a watchdog timer > + implemented by the EL3 Secure Monitor on ARM platforms. > + Requires firmware support. > + To compile this driver as a module, choose M here: the > + module will be called arm_smc_wdt. > + > config LPC18XX_WATCHDOG > tristate "LPC18xx/43xx Watchdog" > depends on ARCH_LPC18XX || COMPILE_TEST > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 6de2e4ceef190..97bed1d3d97cb 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -94,6 +94,7 @@ obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o > obj-$(CONFIG_RTD119X_WATCHDOG) += rtd119x_wdt.o > obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o > obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o > +obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o > > # X86 (i386 + ia64 + x86_64) Architecture > obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o > diff --git a/drivers/watchdog/arm_smc_wdt.c b/drivers/watchdog/arm_smc_wdt.c > new file mode 100644 > index 0000000000000..29d2573b2ca11 > --- /dev/null > +++ b/drivers/watchdog/arm_smc_wdt.c > @@ -0,0 +1,194 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * ARM Secure Monitor Call watchdog driver > + * > + * Copyright 2020 Google LLC. > + * Julius Werner <jwerner@chromium.org> > + * Based on mtk_wdt.c > + */ > + > +#include <linux/arm-smccc.h> > +#include <linux/err.h> > +#include <linux/module.h> > +#include <linux/moduleparam.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/types.h> > +#include <linux/watchdog.h> > +#include <uapi/linux/psci.h> > + > +#define DRV_NAME "arm_smc_wdt" > +#define DRV_VERSION "1.0" > + > +#define get_smc_func_id(wdd) (*(u32 *)watchdog_get_drvdata(wdd)) > +enum smcwd_call { > + SMCWD_INIT = 0, > + SMCWD_SET_TIMEOUT = 1, > + SMCWD_ENABLE = 2, > + SMCWD_PET = 3, > + SMCWD_GET_TIMELEFT = 4, > +}; > + > +static bool nowayout = WATCHDOG_NOWAYOUT; > +static unsigned int timeout; > + > +static int smcwd_call(unsigned long smc_func_id, enum smcwd_call call, > + unsigned long arg, struct arm_smccc_res *res) > +{ > + struct arm_smccc_res local_res; > + > + if (!res) > + res = &local_res; > + > + arm_smccc_smc(smc_func_id, call, arg, 0, 0, 0, 0, 0, res); > + > + if (res->a0 == PSCI_RET_NOT_SUPPORTED) > + return -ENODEV; > + if (res->a0 == PSCI_RET_INVALID_PARAMS) > + return -EINVAL; > + if (res->a0 != PSCI_RET_SUCCESS) > + return -EIO; > + return 0; > +} > + > +static int smcwd_ping(struct watchdog_device *wdd) > +{ > + return smcwd_call(get_smc_func_id(wdd), SMCWD_PET, 0, NULL); > +} > + > +static unsigned int smcwd_get_timeleft(struct watchdog_device *wdd) > +{ > + struct arm_smccc_res res; > + > + smcwd_call(get_smc_func_id(wdd), SMCWD_GET_TIMELEFT, 0, &res); > + return res.a1;It should return 0 when the smcwd_call return error according to Guenter's suggestion at [0] [0]: https://patchwork.kernel.org/patch/11197781/ Thanks. > + > +static int smcwd_set_timeout(struct watchdog_device *wdd, unsigned int timeout) > +{ > + int res; > + > + res = smcwd_call(get_smc_func_id(wdd), SMCWD_SET_TIMEOUT, timeout, > + NULL); > + if (!res) > + wdd->timeout = timeout; > + return res; > +} > + > +static int smcwd_stop(struct watchdog_device *wdd) > +{ > + return smcwd_call(get_smc_func_id(wdd), SMCWD_ENABLE, 0, NULL); > +} > + [...] > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Julius Werner <jwerner@chromium.org>"); > +MODULE_DESCRIPTION("ARM Secure Monitor Call Watchdog Driver"); > +MODULE_VERSION(DRV_VERSION); >
Hi,Evan On 2020/4/22 9:39, Evan Benn wrote: > On Wed, Apr 22, 2020 at 6:31 AM Julius Werner <jwerner@chromium.org> wrote: >> >>> +static int smcwd_call(unsigned long smc_func_id, enum smcwd_call call, >>> + unsigned long arg, struct arm_smccc_res *res) >> >> I think you should just take a struct watchdog_device* here and do the >> drvdata unpacking inside the function. > > That makes sense, I avoided it because smcwd_call's are made during > 'probe', ~while > we are 'constructing' the wdd. But this is C, so I think I have > permission to do this! > >>> +static int smcwd_probe(struct platform_device *pdev) >>> +{ >>> + struct watchdog_device *wdd; >>> + int err; >>> + struct arm_smccc_res res; >>> + u32 *smc_func_id; >>> + >>> + smc_func_id = >>> + devm_kzalloc(&pdev->dev, sizeof(*smc_func_id), GFP_KERNEL); >>> + if (!smc_func_id) >>> + return -ENOMEM; >> >> nit: Could save the allocation by just casting the value itself to a >> pointer? Or is that considered too hacky? > > I am not yet used to what hacks are allowed in the kernel. > Where I learned C that would not be allowed. > I assumed the kernel allocator has fast paths for tiny sizes though. > >>> +static const struct of_device_id smcwd_dt_ids[] = { >>> + { .compatible = "mediatek,mt8173-smc-wdt" }, >>> + {} >>> +}; >>> +MODULE_DEVICE_TABLE(of, smcwd_dt_ids); >> >> So I'm a bit confused about this... I thought the plan was to either >> use arm,smc-id and then there'll be no reason to put platform-specific >> quirks into the driver, so we can just use a generic "arm,smc-wdt" >> compatible string on all platforms; or we put individual compatible >> strings for each platform and use them to hardcode platform-specific >> differences (like the SMC ID) in the driver. But now you're kinda >> doing both by making the driver code platform-independent but still >> using a platform-specific compatible string, that doesn't seem to fit >> together. (If the driver can be platform independent, I think it's >> nicer to have a generic compatible string so that future platforms >> which support the same interface don't have to land code changes in >> order to just use the driver.) > > Yes I think you are correct. I got some reviews about the compatible name, > but I think I misinterpreted those, and arm,smc-wdt would work. I did wonder > if Xingyu from amlogic needed to modify the driver more, EG with different > SMCWD_enum values for the amlogic chip, he could then just add an > amlogic compatible > and keep our devices running with the other compatible. Although of > course it would be nicer if > the amlogic firmware could copy the values here. Using DTS property(arm,smc-id) or vendor's compatible to specify the Function ID is available for the meson-A1.The generic "arm, smc-wdt" looks nicer for MTK and Amlogic sec wdt, but the driver may not cover the other vendor's platform-specific differences in the future, so the platform-specific compatible string may be introduced eventually. But again, I can accept the two methods above. Thanks > > Thanks > > Evan > > . >
> > Yes I think you are correct. I got some reviews about the compatible name, > > but I think I misinterpreted those, and arm,smc-wdt would work. I did wonder > > if Xingyu from amlogic needed to modify the driver more, EG with different > > SMCWD_enum values for the amlogic chip, he could then just add an > > amlogic compatible > > and keep our devices running with the other compatible. Although of > > course it would be nicer if > > the amlogic firmware could copy the values here. > Using DTS property(arm,smc-id) or vendor's compatible to specify the > Function ID is available for the meson-A1.The generic "arm, smc-wdt" > looks nicer for MTK and Amlogic sec wdt, but the driver may not cover > the other vendor's platform-specific differences in the future, so the > platform-specific compatible string may be introduced eventually. I think having a shared driver is only really useful if firmware sticks to the same API anyway. Once we start implementing special cases where every vendor uses a different SMC API, we might as well make that totally different drivers then. For now it sounds like we can fit existing MediaTek Chromebooks and Meson into the same API, so let's do that (with a single arm,smc-wdt compatible string) and then hopefully future vendors will see this interface and design their firmware APIs to match it from the get go.
diff --git a/MAINTAINERS b/MAINTAINERS index 0f2b39767bfa9..2b782bbff200a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1462,6 +1462,7 @@ M: Julius Werner <jwerner@chromium.org> R: Evan Benn <evanbenn@chromium.org> S: Maintained F: devicetree/bindings/watchdog/arm-smc-wdt.yaml +F: drivers/watchdog/arm_smc_wdt.c ARM SMMU DRIVERS M: Will Deacon <will@kernel.org> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index 24e534d850454..0619df80f7575 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -513,6 +513,7 @@ CONFIG_UNIPHIER_THERMAL=y CONFIG_WATCHDOG=y CONFIG_ARM_SP805_WATCHDOG=y CONFIG_ARM_SBSA_WATCHDOG=y +CONFIG_ARM_SMC_WATCHDOG=y CONFIG_S3C2410_WATCHDOG=y CONFIG_DW_WATCHDOG=y CONFIG_SUNXI_WATCHDOG=m diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 0663c604bd642..c440b576d23bf 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -867,6 +867,19 @@ config DIGICOLOR_WATCHDOG To compile this driver as a module, choose M here: the module will be called digicolor_wdt. +config ARM_SMC_WATCHDOG + tristate "ARM Secure Monitor Call based watchdog support" + depends on ARM || ARM64 + depends on OF + depends on HAVE_ARM_SMCCC + select WATCHDOG_CORE + help + Say Y here to include support for a watchdog timer + implemented by the EL3 Secure Monitor on ARM platforms. + Requires firmware support. + To compile this driver as a module, choose M here: the + module will be called arm_smc_wdt. + config LPC18XX_WATCHDOG tristate "LPC18xx/43xx Watchdog" depends on ARCH_LPC18XX || COMPILE_TEST diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 6de2e4ceef190..97bed1d3d97cb 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -94,6 +94,7 @@ obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o obj-$(CONFIG_RTD119X_WATCHDOG) += rtd119x_wdt.o obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o +obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o # X86 (i386 + ia64 + x86_64) Architecture obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o diff --git a/drivers/watchdog/arm_smc_wdt.c b/drivers/watchdog/arm_smc_wdt.c new file mode 100644 index 0000000000000..29d2573b2ca11 --- /dev/null +++ b/drivers/watchdog/arm_smc_wdt.c @@ -0,0 +1,194 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * ARM Secure Monitor Call watchdog driver + * + * Copyright 2020 Google LLC. + * Julius Werner <jwerner@chromium.org> + * Based on mtk_wdt.c + */ + +#include <linux/arm-smccc.h> +#include <linux/err.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/types.h> +#include <linux/watchdog.h> +#include <uapi/linux/psci.h> + +#define DRV_NAME "arm_smc_wdt" +#define DRV_VERSION "1.0" + +#define get_smc_func_id(wdd) (*(u32 *)watchdog_get_drvdata(wdd)) +enum smcwd_call { + SMCWD_INIT = 0, + SMCWD_SET_TIMEOUT = 1, + SMCWD_ENABLE = 2, + SMCWD_PET = 3, + SMCWD_GET_TIMELEFT = 4, +}; + +static bool nowayout = WATCHDOG_NOWAYOUT; +static unsigned int timeout; + +static int smcwd_call(unsigned long smc_func_id, enum smcwd_call call, + unsigned long arg, struct arm_smccc_res *res) +{ + struct arm_smccc_res local_res; + + if (!res) + res = &local_res; + + arm_smccc_smc(smc_func_id, call, arg, 0, 0, 0, 0, 0, res); + + if (res->a0 == PSCI_RET_NOT_SUPPORTED) + return -ENODEV; + if (res->a0 == PSCI_RET_INVALID_PARAMS) + return -EINVAL; + if (res->a0 != PSCI_RET_SUCCESS) + return -EIO; + return 0; +} + +static int smcwd_ping(struct watchdog_device *wdd) +{ + return smcwd_call(get_smc_func_id(wdd), SMCWD_PET, 0, NULL); +} + +static unsigned int smcwd_get_timeleft(struct watchdog_device *wdd) +{ + struct arm_smccc_res res; + + smcwd_call(get_smc_func_id(wdd), SMCWD_GET_TIMELEFT, 0, &res); + return res.a1; +} + +static int smcwd_set_timeout(struct watchdog_device *wdd, unsigned int timeout) +{ + int res; + + res = smcwd_call(get_smc_func_id(wdd), SMCWD_SET_TIMEOUT, timeout, + NULL); + if (!res) + wdd->timeout = timeout; + return res; +} + +static int smcwd_stop(struct watchdog_device *wdd) +{ + return smcwd_call(get_smc_func_id(wdd), SMCWD_ENABLE, 0, NULL); +} + +static int smcwd_start(struct watchdog_device *wdd) +{ + return smcwd_call(get_smc_func_id(wdd), SMCWD_ENABLE, 1, NULL); +} + +static const struct watchdog_info smcwd_info = { + .identity = DRV_NAME, + .options = WDIOF_SETTIMEOUT | + WDIOF_KEEPALIVEPING | + WDIOF_MAGICCLOSE, +}; + +static const struct watchdog_ops smcwd_ops = { + .start = smcwd_start, + .stop = smcwd_stop, + .ping = smcwd_ping, + .set_timeout = smcwd_set_timeout, +}; + +static const struct watchdog_ops smcwd_timeleft_ops = { + .start = smcwd_start, + .stop = smcwd_stop, + .ping = smcwd_ping, + .set_timeout = smcwd_set_timeout, + .get_timeleft = smcwd_get_timeleft, +}; + +static int smcwd_probe(struct platform_device *pdev) +{ + struct watchdog_device *wdd; + int err; + struct arm_smccc_res res; + u32 *smc_func_id; + + smc_func_id = + devm_kzalloc(&pdev->dev, sizeof(*smc_func_id), GFP_KERNEL); + if (!smc_func_id) + return -ENOMEM; + + err = of_property_read_u32(pdev->dev.of_node, "arm,smc-id", + smc_func_id); + if (err < 0) + return err; + + err = smcwd_call(*smc_func_id, SMCWD_INIT, 0, &res); + if (err < 0) + return err; + + wdd = devm_kzalloc(&pdev->dev, sizeof(*wdd), GFP_KERNEL); + if (!wdd) + return -ENOMEM; + + platform_set_drvdata(pdev, wdd); + watchdog_set_drvdata(wdd, smc_func_id); + + wdd->info = &smcwd_info; + /* get_timeleft is optional */ + if (smcwd_call(*smc_func_id, SMCWD_GET_TIMELEFT, 0, NULL)) + wdd->ops = &smcwd_ops; + else + wdd->ops = &smcwd_timeleft_ops; + wdd->timeout = res.a2; + wdd->max_timeout = res.a2; + wdd->min_timeout = res.a1; + wdd->parent = &pdev->dev; + + watchdog_stop_on_reboot(wdd); + watchdog_stop_on_unregister(wdd); + watchdog_set_nowayout(wdd, nowayout); + watchdog_init_timeout(wdd, timeout, &pdev->dev); + err = smcwd_set_timeout(wdd, wdd->timeout); + if (err) + return err; + + err = devm_watchdog_register_device(&pdev->dev, wdd); + if (err) + return err; + + dev_info(&pdev->dev, + "Watchdog registered (timeout=%d sec, nowayout=%d)\n", + wdd->timeout, nowayout); + + return 0; +} + +static const struct of_device_id smcwd_dt_ids[] = { + { .compatible = "mediatek,mt8173-smc-wdt" }, + {} +}; +MODULE_DEVICE_TABLE(of, smcwd_dt_ids); + +static struct platform_driver smcwd_driver = { + .probe = smcwd_probe, + .driver = { + .name = DRV_NAME, + .of_match_table = smcwd_dt_ids, + }, +}; + +module_platform_driver(smcwd_driver); + +module_param(timeout, uint, 0); +MODULE_PARM_DESC(timeout, "Watchdog heartbeat in seconds"); + +module_param(nowayout, bool, 0); +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Julius Werner <jwerner@chromium.org>"); +MODULE_DESCRIPTION("ARM Secure Monitor Call Watchdog Driver"); +MODULE_VERSION(DRV_VERSION);