* [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.
@ 2020-04-21 11:05 Evan Benn
2020-04-21 11:05 ` [PATCH v2 1/2] dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog Evan Benn
` (2 more replies)
0 siblings, 3 replies; 24+ messages in thread
From: Evan Benn @ 2020-04-21 11:05 UTC (permalink / raw)
To: LKML
Cc: xingyu.chen, jwerner, Evan Benn, Anson Huang, Bjorn Andersson,
Catalin Marinas, David S. Miller, Geert Uytterhoeven,
Greg Kroah-Hartman, Guenter Roeck, Leonard Crestez, Li Yang,
Marcin Juszkiewicz, Matthias Brugger, Mauro Carvalho Chehab,
Olof Johansson, Rob Herring, Rob Herring, Shawn Guo,
Valentin Schneider, Will Deacon, Wim Van Sebroeck, devicetree,
linux-arm-kernel, linux-mediatek, linux-watchdog
This is currently supported in firmware deployed on oak, hana and elm mt8173
chromebook devices. The kernel driver is written to be a generic SMC
watchdog driver.
Arm Trusted Firmware upstreaming review:
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405
Patch to add oak, hana, elm device tree:
https://lore.kernel.org/linux-arm-kernel/20200110073730.213789-1-hsinyi@chromium.org/
I would like to add the device tree support after the above patch is
accepted.
Changes in v4:
- Add arm,smc-id property
- Get smc-id from of property
- Return a1 instead of a0 in timeleft
Changes in v3:
- Change name back to arm
- Add optional get_timeleft op
- change name to arm_smc_wdt
Changes in v2:
- Change name arm > mt8173
- use watchdog_stop_on_reboot
- use watchdog_stop_on_unregister
- use devm_watchdog_register_device
- remove smcwd_shutdown, smcwd_remove
- change error codes
Evan Benn (1):
dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog
Julius Werner (1):
watchdog: Add new arm_smc_wdt watchdog driver
.../bindings/watchdog/arm-smc-wdt.yaml | 36 ++++
MAINTAINERS | 7 +
arch/arm64/configs/defconfig | 1 +
drivers/watchdog/Kconfig | 13 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/arm_smc_wdt.c | 194 ++++++++++++++++++
6 files changed, 252 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
create mode 100644 drivers/watchdog/arm_smc_wdt.c
--
2.26.1.301.g55bc3eb7cb9-goog
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v2 1/2] dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog 2020-04-21 11:05 [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Evan Benn @ 2020-04-21 11:05 ` Evan Benn 2020-04-21 11:05 ` [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn 2020-04-21 14:32 ` [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Guenter Roeck 2 siblings, 0 replies; 24+ messages in thread From: Evan Benn @ 2020-04-21 11:05 UTC (permalink / raw) To: LKML Cc: xingyu.chen, jwerner, Evan Benn, David S. Miller, Greg Kroah-Hartman, Guenter Roeck, Matthias Brugger, Mauro Carvalho Chehab, Rob Herring, Wim Van Sebroeck, devicetree, linux-arm-kernel, linux-mediatek, linux-watchdog This watchdog can be used on ARM systems with a Secure Monitor firmware to forward watchdog operations to firmware via a Secure Monitor Call. Signed-off-by: Evan Benn <evanbenn@chromium.org> --- Changes in v4: - Add arm,smc-id property Changes in v3: - Change name back to arm Changes in v2: - Change name arm > mt8173 .../bindings/watchdog/arm-smc-wdt.yaml | 36 +++++++++++++++++++ MAINTAINERS | 6 ++++ 2 files changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml diff --git a/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml b/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml new file mode 100644 index 0000000000000..5379d9f798d84 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml @@ -0,0 +1,36 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/watchdog/arm-smc-wdt.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ARM Secure Monitor Call based watchdog + +allOf: + - $ref: "watchdog.yaml#" + +maintainers: + - Julius Werner <jwerner@chromium.org> + +properties: + compatible: + enum: + - mediatek,mt8173-smc-wdt + arm,smc-id: + allOf: + - $ref: /schemas/types.yaml#/definitions/uint32 + - default: 0x82003D06 + description: + The ATF smc function id used by the firmware. + +required: + - compatible + +examples: + - | + watchdog { + compatible = "mediatek,mt8173-smc-wdt"; + timeout-sec = <15>; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index b816a453b10eb..0f2b39767bfa9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1457,6 +1457,12 @@ S: Maintained F: Documentation/devicetree/bindings/interrupt-controller/arm,vic.txt F: drivers/irqchip/irq-vic.c +ARM SMC WATCHDOG DRIVER +M: Julius Werner <jwerner@chromium.org> +R: Evan Benn <evanbenn@chromium.org> +S: Maintained +F: devicetree/bindings/watchdog/arm-smc-wdt.yaml + ARM SMMU DRIVERS M: Will Deacon <will@kernel.org> R: Robin Murphy <robin.murphy@arm.com> -- 2.26.1.301.g55bc3eb7cb9-goog ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-04-21 11:05 [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Evan Benn 2020-04-21 11:05 ` [PATCH v2 1/2] dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog Evan Benn @ 2020-04-21 11:05 ` Evan Benn 2020-04-21 14:34 ` Guenter Roeck ` (2 more replies) 2020-04-21 14:32 ` [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Guenter Roeck 2 siblings, 3 replies; 24+ messages in thread From: Evan Benn @ 2020-04-21 11:05 UTC (permalink / raw) To: LKML Cc: xingyu.chen, jwerner, Evan Benn, Guenter Roeck, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Geert Uytterhoeven, Greg Kroah-Hartman, Leonard Crestez, Li Yang, Marcin Juszkiewicz, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Shawn Guo, Valentin Schneider, Will Deacon, Wim Van Sebroeck, linux-arm-kernel, linux-mediatek, linux-watchdog 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; +} + +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); -- 2.26.1.301.g55bc3eb7cb9-goog ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-04-21 11:05 ` [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn @ 2020-04-21 14:34 ` Guenter Roeck 2020-04-21 20:31 ` Julius Werner 2020-04-22 4:02 ` Xingyu Chen 2 siblings, 0 replies; 24+ messages in thread From: Guenter Roeck @ 2020-04-21 14:34 UTC (permalink / raw) To: Evan Benn, LKML Cc: xingyu.chen, jwerner, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Geert Uytterhoeven, Greg Kroah-Hartman, Leonard Crestez, Li Yang, Marcin Juszkiewicz, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Shawn Guo, Valentin Schneider, Will Deacon, Wim Van Sebroeck, linux-arm-kernel, linux-mediatek, linux-watchdog 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); > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-04-21 11:05 ` [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn 2020-04-21 14:34 ` Guenter Roeck @ 2020-04-21 20:31 ` Julius Werner 2020-04-22 1:39 ` Evan Benn 2020-04-22 3:23 ` Guenter Roeck 2020-04-22 4:02 ` Xingyu Chen 2 siblings, 2 replies; 24+ messages in thread From: Julius Werner @ 2020-04-21 20:31 UTC (permalink / raw) To: Evan Benn Cc: LKML, Xingyu Chen, Julius Werner, Guenter Roeck, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Geert Uytterhoeven, Greg Kroah-Hartman, Leonard Crestez, Li Yang, Marcin Juszkiewicz, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Shawn Guo, Valentin Schneider, Will Deacon, Wim Van Sebroeck, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, LINUX-WATCHDOG > +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.) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-04-21 20:31 ` Julius Werner @ 2020-04-22 1:39 ` Evan Benn 2020-04-22 6:02 ` Xingyu Chen 2020-04-22 3:23 ` Guenter Roeck 1 sibling, 1 reply; 24+ messages in thread From: Evan Benn @ 2020-04-22 1:39 UTC (permalink / raw) To: Julius Werner Cc: LKML, Xingyu Chen, Guenter Roeck, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Geert Uytterhoeven, Greg Kroah-Hartman, Leonard Crestez, Li Yang, Marcin Juszkiewicz, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Shawn Guo, Valentin Schneider, Will Deacon, Wim Van Sebroeck, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, LINUX-WATCHDOG 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-04-22 1:39 ` Evan Benn @ 2020-04-22 6:02 ` Xingyu Chen 2020-04-22 22:22 ` Julius Werner 0 siblings, 1 reply; 24+ messages in thread From: Xingyu Chen @ 2020-04-22 6:02 UTC (permalink / raw) To: Evan Benn, Julius Werner Cc: LKML, Guenter Roeck, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Geert Uytterhoeven, Greg Kroah-Hartman, Leonard Crestez, Li Yang, Marcin Juszkiewicz, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Shawn Guo, Valentin Schneider, Will Deacon, Wim Van Sebroeck, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, LINUX-WATCHDOG, Jianxin Pan, Yonghui Yu, Xingyu Chen 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 > > . > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-04-22 6:02 ` Xingyu Chen @ 2020-04-22 22:22 ` Julius Werner 0 siblings, 0 replies; 24+ messages in thread From: Julius Werner @ 2020-04-22 22:22 UTC (permalink / raw) To: Xingyu Chen Cc: Evan Benn, Julius Werner, LKML, Guenter Roeck, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Geert Uytterhoeven, Greg Kroah-Hartman, Leonard Crestez, Li Yang, Marcin Juszkiewicz, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Shawn Guo, Valentin Schneider, Will Deacon, Wim Van Sebroeck, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, LINUX-WATCHDOG, Jianxin Pan, Yonghui Yu > > 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-04-21 20:31 ` Julius Werner 2020-04-22 1:39 ` Evan Benn @ 2020-04-22 3:23 ` Guenter Roeck 1 sibling, 0 replies; 24+ messages in thread From: Guenter Roeck @ 2020-04-22 3:23 UTC (permalink / raw) To: Julius Werner, Evan Benn Cc: LKML, Xingyu Chen, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Geert Uytterhoeven, Greg Kroah-Hartman, Leonard Crestez, Li Yang, Marcin Juszkiewicz, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Shawn Guo, Valentin Schneider, Will Deacon, Wim Van Sebroeck, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, LINUX-WATCHDOG 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.) > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-04-21 11:05 ` [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn 2020-04-21 14:34 ` Guenter Roeck 2020-04-21 20:31 ` Julius Werner @ 2020-04-22 4:02 ` Xingyu Chen 2 siblings, 0 replies; 24+ messages in thread From: Xingyu Chen @ 2020-04-22 4:02 UTC (permalink / raw) To: Evan Benn, LKML Cc: jwerner, Guenter Roeck, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Geert Uytterhoeven, Greg Kroah-Hartman, Leonard Crestez, Li Yang, Marcin Juszkiewicz, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Shawn Guo, Valentin Schneider, Will Deacon, Wim Van Sebroeck, linux-arm-kernel, linux-mediatek, linux-watchdog, Jianxin Pan, Yonghui Yu, Xingyu Chen 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); > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls. 2020-04-21 11:05 [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Evan Benn 2020-04-21 11:05 ` [PATCH v2 1/2] dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog Evan Benn 2020-04-21 11:05 ` [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn @ 2020-04-21 14:32 ` Guenter Roeck 2020-04-22 0:40 ` Evan Benn 2 siblings, 1 reply; 24+ messages in thread From: Guenter Roeck @ 2020-04-21 14:32 UTC (permalink / raw) To: Evan Benn, LKML Cc: xingyu.chen, jwerner, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Geert Uytterhoeven, Greg Kroah-Hartman, Leonard Crestez, Li Yang, Marcin Juszkiewicz, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Rob Herring, Shawn Guo, Valentin Schneider, Will Deacon, Wim Van Sebroeck, devicetree, linux-arm-kernel, linux-mediatek, linux-watchdog On 4/21/20 4:05 AM, Evan Benn wrote: > This is currently supported in firmware deployed on oak, hana and elm mt8173 > chromebook devices. The kernel driver is written to be a generic SMC > watchdog driver. > > Arm Trusted Firmware upstreaming review: > https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405 > > Patch to add oak, hana, elm device tree: > https://lore.kernel.org/linux-arm-kernel/20200110073730.213789-1-hsinyi@chromium.org/ > I would like to add the device tree support after the above patch is > accepted. > > Changes in v4: > - Add arm,smc-id property > - Get smc-id from of property > - Return a1 instead of a0 in timeleft > Subject says v2. This is confusing, at the very least. Guenter > Changes in v3: > - Change name back to arm > - Add optional get_timeleft op > - change name to arm_smc_wdt > > Changes in v2: > - Change name arm > mt8173 > - use watchdog_stop_on_reboot > - use watchdog_stop_on_unregister > - use devm_watchdog_register_device > - remove smcwd_shutdown, smcwd_remove > - change error codes > > Evan Benn (1): > dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog > > Julius Werner (1): > watchdog: Add new arm_smc_wdt watchdog driver > > .../bindings/watchdog/arm-smc-wdt.yaml | 36 ++++ > MAINTAINERS | 7 + > arch/arm64/configs/defconfig | 1 + > drivers/watchdog/Kconfig | 13 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/arm_smc_wdt.c | 194 ++++++++++++++++++ > 6 files changed, 252 insertions(+) > create mode 100644 Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml > create mode 100644 drivers/watchdog/arm_smc_wdt.c > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls. 2020-04-21 14:32 ` [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Guenter Roeck @ 2020-04-22 0:40 ` Evan Benn 0 siblings, 0 replies; 24+ messages in thread From: Evan Benn @ 2020-04-22 0:40 UTC (permalink / raw) To: Guenter Roeck, Simon Glass Cc: LKML, Xingyu Chen, Julius Werner, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Geert Uytterhoeven, Greg Kroah-Hartman, Leonard Crestez, Li Yang, Marcin Juszkiewicz, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Rob Herring, Shawn Guo, Valentin Schneider, Will Deacon, Wim Van Sebroeck, devicetree, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, LINUX-WATCHDOG > Subject says v2. This is confusing, at the very least. > > Guenter Apologies! I am using the patman script, it threw this message that I did not understand: 'Change log for unknown version v3'. And I did not spot the issue in the emails before send. Not sure why patman worked for v2 and v3 but not v4... I will take a look. Evan ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.
@ 2020-04-03 5:28 Evan Benn
2020-04-03 6:08 ` Evan Benn
[not found] ` <CAKz_xw0gV+w_gMkLfB4qUBdULLfFoiv1TBWp9_PHy33wP_XWyA@mail.gmail.com>
0 siblings, 2 replies; 24+ messages in thread
From: Evan Benn @ 2020-04-03 5:28 UTC (permalink / raw)
To: LKML
Cc: xingyu.chen, jwerner, Evan Benn, Andy Shevchenko, Anson Huang,
Bjorn Andersson, Catalin Marinas, David S. Miller,
Greg Kroah-Hartman, Guenter Roeck, Jonathan Cameron,
Leonard Crestez, Manivannan Sadhasivam, Marcin Juszkiewicz,
Mark Rutland, Matthias Brugger, Mauro Carvalho Chehab,
Olof Johansson, Rob Herring, Rob Herring, Shawn Guo,
Valentin Schneider, Vinod Koul, Will Deacon, Wim Van Sebroeck,
devicetree, linux-arm-kernel, linux-mediatek, linux-watchdog
This is currently supported in firmware deployed on oak, hana and elm mt8173
chromebook devices. The kernel driver is written to be a generic SMC
watchdog driver.
Arm Trusted Firmware upstreaming review:
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405
Patch to add oak, hana, elm device tree:
https://lore.kernel.org/linux-arm-kernel/20200110073730.213789-1-hsinyi@chromium.org/
I would like to add the device tree support after the above patch is
accepted.
Changes in v3:
- Change name back to arm
- Add optional get_timeleft op
- change name to arm_smc_wdt
Changes in v2:
- Change name arm > mt8173
- use watchdog_stop_on_reboot
- use watchdog_stop_on_unregister
- use devm_watchdog_register_device
- remove smcwd_shutdown, smcwd_remove
- change error codes
Evan Benn (1):
dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog
Julius Werner (1):
watchdog: Add new arm_smd_wdt watchdog driver
.../bindings/watchdog/arm-smc-wdt.yaml | 30 +++
MAINTAINERS | 7 +
arch/arm64/configs/defconfig | 1 +
drivers/watchdog/Kconfig | 13 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/arm_smc_wdt.c | 181 ++++++++++++++++++
6 files changed, 233 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
create mode 100644 drivers/watchdog/arm_smc_wdt.c
--
2.26.0.292.g33ef6b2f38-goog
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls. 2020-04-03 5:28 Evan Benn @ 2020-04-03 6:08 ` Evan Benn [not found] ` <CAKz_xw0gV+w_gMkLfB4qUBdULLfFoiv1TBWp9_PHy33wP_XWyA@mail.gmail.com> 1 sibling, 0 replies; 24+ messages in thread From: Evan Benn @ 2020-04-03 6:08 UTC (permalink / raw) To: LKML Cc: Xingyu Chen, Julius Werner, Andy Shevchenko, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Greg Kroah-Hartman, Guenter Roeck, Jonathan Cameron, Leonard Crestez, Manivannan Sadhasivam, Marcin Juszkiewicz, Mark Rutland, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Rob Herring, Shawn Guo, Valentin Schneider, Vinod Koul, Will Deacon, Wim Van Sebroeck, devicetree, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, LINUX-WATCHDOG Apologies I forgot to add this note to my cover letter. Xingyu do you mind seeing if you can modify your ATF firmware to match this driver? We can add a compatible or make other changes to suit you. Thanks On Fri, Apr 3, 2020 at 4:29 PM Evan Benn <evanbenn@chromium.org> wrote: > > This is currently supported in firmware deployed on oak, hana and elm mt8173 > chromebook devices. The kernel driver is written to be a generic SMC > watchdog driver. > > Arm Trusted Firmware upstreaming review: > https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405 > > Patch to add oak, hana, elm device tree: > https://lore.kernel.org/linux-arm-kernel/20200110073730.213789-1-hsinyi@chromium.org/ > I would like to add the device tree support after the above patch is > accepted. > > Changes in v3: > - Change name back to arm > - Add optional get_timeleft op > - change name to arm_smc_wdt > > Changes in v2: > - Change name arm > mt8173 > - use watchdog_stop_on_reboot > - use watchdog_stop_on_unregister > - use devm_watchdog_register_device > - remove smcwd_shutdown, smcwd_remove > - change error codes > > Evan Benn (1): > dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog > > Julius Werner (1): > watchdog: Add new arm_smd_wdt watchdog driver > > .../bindings/watchdog/arm-smc-wdt.yaml | 30 +++ > MAINTAINERS | 7 + > arch/arm64/configs/defconfig | 1 + > drivers/watchdog/Kconfig | 13 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/arm_smc_wdt.c | 181 ++++++++++++++++++ > 6 files changed, 233 insertions(+) > create mode 100644 Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml > create mode 100644 drivers/watchdog/arm_smc_wdt.c > > -- > 2.26.0.292.g33ef6b2f38-goog > ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <CAKz_xw0gV+w_gMkLfB4qUBdULLfFoiv1TBWp9_PHy33wP_XWyA@mail.gmail.com>]
[parent not found: <890948ef-7276-fdae-d270-eb30eff3eab2@amlogic.com>]
* Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls. [not found] ` <890948ef-7276-fdae-d270-eb30eff3eab2@amlogic.com> @ 2020-04-15 11:54 ` Xingyu Chen 2020-04-15 22:29 ` Julius Werner 0 siblings, 1 reply; 24+ messages in thread From: Xingyu Chen @ 2020-04-15 11:54 UTC (permalink / raw) To: Evan Benn, LKML Cc: Julius Werner, Andy Shevchenko, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Greg Kroah-Hartman, Guenter Roeck, Jonathan Cameron, Leonard Crestez, Manivannan Sadhasivam, Marcin Juszkiewicz, Mark Rutland, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Rob Herring, Shawn Guo, Valentin Schneider, Vinod Koul, Will Deacon, Wim Van Sebroeck, devicetree, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, LINUX-WATCHDOG, Jianxin Pan, Yonghui Yu, open list:ARM/Amlogic Meson..., Xingyu Chen Hi,Evan On 2020/4/11 23:06, Xingyu Chen wrote: > Hi, Evan > > On 2020/4/3 14:04, Evan Benn wrote: >> Apologies I forgot to add this note to my cover letter. >> >> Xingyu do you mind seeing if you can modify your ATF firmware to match >> this driver? >> We can add a compatible or make other changes to suit you. > Thanks for your patch [0], I will test this patch on the meson-A1 > platform, but It looks more > convenient to be compatible with other platforms if using the compatible > strings to correlate > platform differences include function ID and wdt_ops. > > [0]: https://patchwork.kernel.org/patch/11471829/ I have tested your patch on the meson-A1, but I use the compatible strings to correlate the following platform differences，it works normally. static const struct smcwd_data smcwd_mtk_data = { .func_id = 0x82003d06, .ops = &smcwd_ops, } static const struct smcwd_data smcwd_meson_data = { .func_id = 0x82000086, .ops = &smcwd_timeleft_ops, } In addition, It looks more reasonable to use the "msec" as the unit of timeout parameter for the ATF fw interface with SMCWD_SET_TIMEOUT: - The fw interface will compatible with the uboot generic watchdog interface at [0], and there is no need to convert timeout from msec to sec. - Some vendor's watchdog may be not support the "wdt_trigger_reset" reset operation, but they can use the method below to reset the system by the watchdog right now. watchdog_set_time(1); //1ms watchdog_enable(); [0]: https://gitlab.denx.de/u-boot/u-boot/-/blob/master/drivers/watchdog/wdt-uclass.c Best Regards >> Thanks >> >> On Fri, Apr 3, 2020 at 4:29 PM Evan Benn <evanbenn@chromium.org >> <mailto:evanbenn@chromium.org>> wrote: >> >> This is currently supported in firmware deployed on oak, hana and >> elm mt8173 >> chromebook devices. The kernel driver is written to be a generic SMC >> watchdog driver. >> >> Arm Trusted Firmware upstreaming review: >> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405 >> >> Patch to add oak, hana, elm device tree: >> https://lore.kernel.org/linux-arm-kernel/20200110073730.213789-1-hsinyi@chromium.org/ >> I would like to add the device tree support after the above patch is >> accepted. >> >> Changes in v3: >> - Change name back to arm >> - Add optional get_timeleft op >> - change name to arm_smc_wdt >> >> Changes in v2: >> - Change name arm > mt8173 >> - use watchdog_stop_on_reboot >> - use watchdog_stop_on_unregister >> - use devm_watchdog_register_device >> - remove smcwd_shutdown, smcwd_remove >> - change error codes >> >> Evan Benn (1): >> dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog >> >> Julius Werner (1): >> watchdog: Add new arm_smd_wdt watchdog driver >> >> .../bindings/watchdog/arm-smc-wdt.yaml | 30 +++ >> MAINTAINERS | 7 + >> arch/arm64/configs/defconfig | 1 + >> drivers/watchdog/Kconfig | 13 ++ >> drivers/watchdog/Makefile | 1 + >> drivers/watchdog/arm_smc_wdt.c | 181 >> ++++++++++++++++++ >> 6 files changed, 233 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml >> create mode 100644 drivers/watchdog/arm_smc_wdt.c >> >> -- >> 2.26.0.292.g33ef6b2f38-goog >> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls. 2020-04-15 11:54 ` Xingyu Chen @ 2020-04-15 22:29 ` Julius Werner 2020-04-15 23:12 ` Guenter Roeck 2020-04-16 3:23 ` Xingyu Chen 0 siblings, 2 replies; 24+ messages in thread From: Julius Werner @ 2020-04-15 22:29 UTC (permalink / raw) To: Xingyu Chen Cc: Evan Benn, LKML, Julius Werner, Andy Shevchenko, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Greg Kroah-Hartman, Guenter Roeck, Jonathan Cameron, Leonard Crestez, Manivannan Sadhasivam, Marcin Juszkiewicz, Mark Rutland, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Rob Herring, Shawn Guo, Valentin Schneider, Vinod Koul, Will Deacon, Wim Van Sebroeck, devicetree, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, LINUX-WATCHDOG, Jianxin Pan, Yonghui Yu, open list:ARM/Amlogic Meson... > In addition, It looks more reasonable to use the "msec" as the unit of > timeout parameter for the ATF fw interface with SMCWD_SET_TIMEOUT: > > - The fw interface will compatible with the uboot generic watchdog > interface at [0], and there is no need to convert timeout from msec > to sec. I think we're trying hard to keep this compatible to a Trusted Firmware counterpart that we have already shipped, so we would prefer to keep it at seconds if possible. That's what the Linux watchdog core uses as well after all, so it just seems natural. I don't really see how what U-Boot does would have anything to do with this. > - Some vendor's watchdog may be not support the "wdt_trigger_reset" > reset operation, but they can use the method below to reset the system > by the watchdog right now. > > watchdog_set_time(1); //1ms > watchdog_enable(); They can still do that but they should do that on the Trusted Firmware side. Emulating a missing reset functionality should be handled by the hardware abstraction layer (in this case Trusted Firmware), not at the Linux API level. So Linux would still send a PSCI_SYSTEM_RESET SMC, but then Trusted Firmware can choose to implement that by setting the watchdog to the smallest possible timeout (which it can because it's accessing it directly, not through this SMC interface) and letting it expire. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls. 2020-04-15 22:29 ` Julius Werner @ 2020-04-15 23:12 ` Guenter Roeck 2020-04-16 0:46 ` Evan Benn 2020-04-16 3:23 ` Xingyu Chen 1 sibling, 1 reply; 24+ messages in thread From: Guenter Roeck @ 2020-04-15 23:12 UTC (permalink / raw) To: Julius Werner Cc: Xingyu Chen, Evan Benn, LKML, Andy Shevchenko, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Greg Kroah-Hartman, Jonathan Cameron, Leonard Crestez, Manivannan Sadhasivam, Marcin Juszkiewicz, Mark Rutland, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Rob Herring, Shawn Guo, Valentin Schneider, Vinod Koul, Will Deacon, Wim Van Sebroeck, devicetree, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, LINUX-WATCHDOG, Jianxin Pan, Yonghui Yu, open list:ARM/Amlogic Meson... On Wed, Apr 15, 2020 at 03:29:29PM -0700, Julius Werner wrote: > > In addition, It looks more reasonable to use the "msec" as the unit of > > timeout parameter for the ATF fw interface with SMCWD_SET_TIMEOUT: > > > > - The fw interface will compatible with the uboot generic watchdog > > interface at [0], and there is no need to convert timeout from msec > > to sec. > > I think we're trying hard to keep this compatible to a Trusted > Firmware counterpart that we have already shipped, so we would prefer > to keep it at seconds if possible. That's what the Linux watchdog core > uses as well after all, so it just seems natural. I don't really see > how what U-Boot does would have anything to do with this. > > > - Some vendor's watchdog may be not support the "wdt_trigger_reset" > > reset operation, but they can use the method below to reset the system > > by the watchdog right now. > > > > watchdog_set_time(1); //1ms > > watchdog_enable(); > > They can still do that but they should do that on the Trusted Firmware > side. Emulating a missing reset functionality should be handled by the > hardware abstraction layer (in this case Trusted Firmware), not at the > Linux API level. So Linux would still send a PSCI_SYSTEM_RESET SMC, > but then Trusted Firmware can choose to implement that by setting the > watchdog to the smallest possible timeout (which it can because it's > accessing it directly, not through this SMC interface) and letting it > expire. I agree. Using a watchdog to implement reset functionality is always a means of last resort and should be avoided if possible. Guenter ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls. 2020-04-15 23:12 ` Guenter Roeck @ 2020-04-16 0:46 ` Evan Benn 2020-04-16 0:56 ` Julius Werner 0 siblings, 1 reply; 24+ messages in thread From: Evan Benn @ 2020-04-16 0:46 UTC (permalink / raw) To: Guenter Roeck Cc: Julius Werner, Xingyu Chen, LKML, Andy Shevchenko, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Greg Kroah-Hartman, Jonathan Cameron, Leonard Crestez, Manivannan Sadhasivam, Marcin Juszkiewicz, Mark Rutland, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Rob Herring, Shawn Guo, Valentin Schneider, Vinod Koul, Will Deacon, Wim Van Sebroeck, devicetree, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, LINUX-WATCHDOG, Jianxin Pan, Yonghui Yu, open list:ARM/Amlogic Meson... Thanks Xingyu, Can anyone provide advice about making SMCWD_FUNC_ID a device tree param directly, vs using the compatible to select from a table. Please note get_timeleft erroneously returns res.a0 instead of res.a1 in this version. Evan On Thu, Apr 16, 2020 at 9:12 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On Wed, Apr 15, 2020 at 03:29:29PM -0700, Julius Werner wrote: > > > In addition, It looks more reasonable to use the "msec" as the unit of > > > timeout parameter for the ATF fw interface with SMCWD_SET_TIMEOUT: > > > > > > - The fw interface will compatible with the uboot generic watchdog > > > interface at [0], and there is no need to convert timeout from msec > > > to sec. > > > > I think we're trying hard to keep this compatible to a Trusted > > Firmware counterpart that we have already shipped, so we would prefer > > to keep it at seconds if possible. That's what the Linux watchdog core > > uses as well after all, so it just seems natural. I don't really see > > how what U-Boot does would have anything to do with this. > > > > > - Some vendor's watchdog may be not support the "wdt_trigger_reset" > > > reset operation, but they can use the method below to reset the system > > > by the watchdog right now. > > > > > > watchdog_set_time(1); //1ms > > > watchdog_enable(); > > > > They can still do that but they should do that on the Trusted Firmware > > side. Emulating a missing reset functionality should be handled by the > > hardware abstraction layer (in this case Trusted Firmware), not at the > > Linux API level. So Linux would still send a PSCI_SYSTEM_RESET SMC, > > but then Trusted Firmware can choose to implement that by setting the > > watchdog to the smallest possible timeout (which it can because it's > > accessing it directly, not through this SMC interface) and letting it > > expire. > > I agree. Using a watchdog to implement reset functionality is always a > means of last resort and should be avoided if possible. > > Guenter ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls. 2020-04-16 0:46 ` Evan Benn @ 2020-04-16 0:56 ` Julius Werner 2020-04-16 3:48 ` Florian Fainelli 0 siblings, 1 reply; 24+ messages in thread From: Julius Werner @ 2020-04-16 0:56 UTC (permalink / raw) To: Evan Benn Cc: Guenter Roeck, Julius Werner, Xingyu Chen, LKML, Andy Shevchenko, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Greg Kroah-Hartman, Jonathan Cameron, Leonard Crestez, Manivannan Sadhasivam, Marcin Juszkiewicz, Mark Rutland, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Rob Herring, Shawn Guo, Valentin Schneider, Vinod Koul, Will Deacon, Wim Van Sebroeck, devicetree, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, LINUX-WATCHDOG, Jianxin Pan, Yonghui Yu, open list:ARM/Amlogic Meson... > Can anyone provide advice about making SMCWD_FUNC_ID a device tree > param directly, vs using the compatible to select from a table. Sounds like most people prefer the way with using different compatible strings? (Personally I don't really care either way.) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls. 2020-04-16 0:56 ` Julius Werner @ 2020-04-16 3:48 ` Florian Fainelli 2020-04-21 1:08 ` Evan Benn 0 siblings, 1 reply; 24+ messages in thread From: Florian Fainelli @ 2020-04-16 3:48 UTC (permalink / raw) To: Julius Werner, Evan Benn Cc: Mark Rutland, Catalin Marinas, Bjorn Andersson, Manivannan Sadhasivam, Yonghui Yu, Leonard Crestez, Will Deacon, Xingyu Chen, Rob Herring, Wim Van Sebroeck, Anson Huang, Mauro Carvalho Chehab, Marcin Juszkiewicz, Valentin Schneider, Guenter Roeck, devicetree, LINUX-WATCHDOG, Rob Herring, moderated list:ARM/Mediatek SoC support, Jonathan Cameron, Matthias Brugger, open list:ARM/Amlogic Meson..., Andy Shevchenko, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Jianxin Pan, Greg Kroah-Hartman, LKML, Vinod Koul, Olof Johansson, Shawn Guo, David S. Miller On 4/15/2020 5:56 PM, Julius Werner wrote: >> Can anyone provide advice about making SMCWD_FUNC_ID a device tree >> param directly, vs using the compatible to select from a table. > > Sounds like most people prefer the way with using different compatible > strings? (Personally I don't really care either way.) The PSCI binding itself has provision for specifying function IDs for different functions, and this seems to be followed by other subsystems as well like SCMI: https://www.spinics.net/lists/arm-kernel/msg791270.html I could easily imagine that a firmware would provide two functions IDs (one for AArch32, one for AArch64) and that it could change those over time while not changing the compatible string at all. -- Florian ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls. 2020-04-16 3:48 ` Florian Fainelli @ 2020-04-21 1:08 ` Evan Benn 2020-04-21 2:36 ` Florian Fainelli 0 siblings, 1 reply; 24+ messages in thread From: Evan Benn @ 2020-04-21 1:08 UTC (permalink / raw) To: Florian Fainelli Cc: Julius Werner, Mark Rutland, Catalin Marinas, Bjorn Andersson, Manivannan Sadhasivam, Yonghui Yu, Leonard Crestez, Will Deacon, Xingyu Chen, Rob Herring, Wim Van Sebroeck, Anson Huang, Mauro Carvalho Chehab, Marcin Juszkiewicz, Valentin Schneider, Guenter Roeck, devicetree, LINUX-WATCHDOG, Rob Herring, moderated list:ARM/Mediatek SoC support, Jonathan Cameron, Matthias Brugger, open list:ARM/Amlogic Meson..., Andy Shevchenko, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Jianxin Pan, Greg Kroah-Hartman, LKML, Vinod Koul, Olof Johansson, Shawn Guo, David S. Miller Thanks Florian, > The PSCI binding itself has provision for specifying function IDs for > different functions, and this seems to be followed by other subsystems > as well like SCMI: > > https://www.spinics.net/lists/arm-kernel/msg791270.html Are you referring to this line in the devicetree linked? +- arm,smc-id : SMC id required when using smc or hvc transports I cannot find any prior definition of this in the devicetree yaml format, so I will add that as well. Did you have a link for the psci usage that you referenced? Thanks Evan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls. 2020-04-21 1:08 ` Evan Benn @ 2020-04-21 2:36 ` Florian Fainelli 0 siblings, 0 replies; 24+ messages in thread From: Florian Fainelli @ 2020-04-21 2:36 UTC (permalink / raw) To: Evan Benn Cc: Julius Werner, Mark Rutland, Catalin Marinas, Bjorn Andersson, Manivannan Sadhasivam, Yonghui Yu, Leonard Crestez, Will Deacon, Xingyu Chen, Rob Herring, Wim Van Sebroeck, Anson Huang, Mauro Carvalho Chehab, Marcin Juszkiewicz, Valentin Schneider, Guenter Roeck, devicetree, LINUX-WATCHDOG, Rob Herring, moderated list:ARM/Mediatek SoC support, Jonathan Cameron, Matthias Brugger, open list:ARM/Amlogic Meson..., Andy Shevchenko, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Jianxin Pan, Greg Kroah-Hartman, LKML, Vinod Koul, Olof Johansson, Shawn Guo, David S. Miller On 4/20/2020 6:08 PM, Evan Benn wrote: > Thanks Florian, > >> The PSCI binding itself has provision for specifying function IDs for >> different functions, and this seems to be followed by other subsystems >> as well like SCMI: >> >> https://www.spinics.net/lists/arm-kernel/msg791270.html > > Are you referring to this line in the devicetree linked? > > +- arm,smc-id : SMC id required when using smc or hvc transports > > I cannot find any prior definition of this in the devicetree yaml > format, so I will add that as well. > Did you have a link for the psci usage that you referenced? Sure, line 80 and below from psci.yaml: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/arm/psci.yaml#n80 -- Florian ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls. 2020-04-15 22:29 ` Julius Werner 2020-04-15 23:12 ` Guenter Roeck @ 2020-04-16 3:23 ` Xingyu Chen 1 sibling, 0 replies; 24+ messages in thread From: Xingyu Chen @ 2020-04-16 3:23 UTC (permalink / raw) To: Julius Werner Cc: Evan Benn, LKML, Andy Shevchenko, Anson Huang, Bjorn Andersson, Catalin Marinas, David S. Miller, Greg Kroah-Hartman, Guenter Roeck, Jonathan Cameron, Leonard Crestez, Manivannan Sadhasivam, Marcin Juszkiewicz, Mark Rutland, Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson, Rob Herring, Rob Herring, Shawn Guo, Valentin Schneider, Vinod Koul, Will Deacon, Wim Van Sebroeck, devicetree, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, LINUX-WATCHDOG, Jianxin Pan, Yonghui Yu, open list:ARM/Amlogic Meson... Hi,Julius On 2020/4/16 6:29, Julius Werner wrote: >> In addition, It looks more reasonable to use the "msec" as the unit of >> timeout parameter for the ATF fw interface with SMCWD_SET_TIMEOUT: >> >> - The fw interface will compatible with the uboot generic watchdog >> interface at [0], and there is no need to convert timeout from msec >> to sec. > > I think we're trying hard to keep this compatible to a Trusted > Firmware counterpart that we have already shipped, so we would prefer > to keep it at seconds if possible. That's what the Linux watchdog core > uses as well after all, so it just seems natural. I don't really see > how what U-Boot does would have anything to do with this. If the uboot introduces a smcwd driver, and it maybe work like this: static const struct wdt_ops XXX_wdt_ops = { .start = XXX_wdt_start, ... } static int XXX_wdt_start(struct udevice *dev, u64 ms, ulong flags) { timeout = ms / 1000; //convert timeout from msec to sec. smcwd_call(SMCWD_SET_TIMEOUT, timeout, NULL); smcwd_call(SMCWD_ENABLE, 0, NULL); } Best Regards > >> - Some vendor's watchdog may be not support the "wdt_trigger_reset" >> reset operation, but they can use the method below to reset the system >> by the watchdog right now. >> >> watchdog_set_time(1); //1ms >> watchdog_enable(); > > They can still do that but they should do that on the Trusted Firmware > side. Emulating a missing reset functionality should be handled by the > hardware abstraction layer (in this case Trusted Firmware), not at the > Linux API level. So Linux would still send a PSCI_SYSTEM_RESET SMC, > but then Trusted Firmware can choose to implement that by setting the > watchdog to the smallest possible timeout (which it can because it's > accessing it directly, not through this SMC interface) and letting it > expire. > > . > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.
@ 2020-02-21 5:38 Evan Benn
0 siblings, 0 replies; 24+ messages in thread
From: Evan Benn @ 2020-02-21 5:38 UTC (permalink / raw)
To: LKML
Cc: jwerner, Evan Benn, Anson Huang, Catalin Marinas,
Clément Péron, David S. Miller, Dinh Nguyen,
Greg Kroah-Hartman, Guenter Roeck, Jonathan Cameron,
Leonard Crestez, Marcin Juszkiewicz, Mark Rutland,
Matthias Brugger, Mauro Carvalho Chehab, Olof Johansson,
Rob Herring, Rob Herring, Shawn Guo, Will Deacon,
Wim Van Sebroeck, devicetree, linux-arm-kernel, linux-mediatek,
linux-watchdog
This is currently supported in firmware deployed on oak, hana and elm mt8173
chromebook devices. The kernel driver is written to be a generic SMC
watchdog driver.
Arm Trusted Firmware upstreaming review:
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405
Patch to add oak, hana, elm device tree:
https://lore.kernel.org/linux-arm-kernel/20200110073730.213789-1-hsinyi@chromium.org/
I would like to add the device tree support after the above patch is
accepted.
Changes in v2:
- Change name arm > mt8173
- use watchdog_stop_on_reboot
- use watchdog_stop_on_unregister
- use devm_watchdog_register_device
- remove smcwd_shutdown, smcwd_remove
- change error codes
Evan Benn (1):
dt-bindings: watchdog: Add mt8173,smc-wdt watchdog
Julius Werner (1):
watchdog: Add new mt8173_smc_wdt watchdog driver
.../bindings/watchdog/mt8173,smc-wdt.yaml | 30 ++++
MAINTAINERS | 7 +
arch/arm64/configs/defconfig | 1 +
drivers/watchdog/Kconfig | 13 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/mt8173_smc_wdt.c | 160 ++++++++++++++++++
6 files changed, 212 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/mt8173,smc-wdt.yaml
create mode 100644 drivers/watchdog/mt8173_smc_wdt.c
--
2.25.0.265.gbab2e86ba0-goog
^ permalink raw reply [flat|nested] 24+ messages in threadend of thread, other threads:[~2020-04-22 22:22 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-21 11:05 [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Evan Benn
2020-04-21 11:05 ` [PATCH v2 1/2] dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog Evan Benn
2020-04-21 11:05 ` [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn
2020-04-21 14:34 ` Guenter Roeck
2020-04-21 20:31 ` Julius Werner
2020-04-22 1:39 ` Evan Benn
2020-04-22 6:02 ` Xingyu Chen
2020-04-22 22:22 ` Julius Werner
2020-04-22 3:23 ` Guenter Roeck
2020-04-22 4:02 ` Xingyu Chen
2020-04-21 14:32 ` [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Guenter Roeck
2020-04-22 0:40 ` Evan Benn
-- strict thread matches above, loose matches on Subject: below --
2020-04-03 5:28 Evan Benn
2020-04-03 6:08 ` Evan Benn
[not found] ` <CAKz_xw0gV+w_gMkLfB4qUBdULLfFoiv1TBWp9_PHy33wP_XWyA@mail.gmail.com>
[not found] ` <890948ef-7276-fdae-d270-eb30eff3eab2@amlogic.com>
2020-04-15 11:54 ` Xingyu Chen
2020-04-15 22:29 ` Julius Werner
2020-04-15 23:12 ` Guenter Roeck
2020-04-16 0:46 ` Evan Benn
2020-04-16 0:56 ` Julius Werner
2020-04-16 3:48 ` Florian Fainelli
2020-04-21 1:08 ` Evan Benn
2020-04-21 2:36 ` Florian Fainelli
2020-04-16 3:23 ` Xingyu Chen
2020-02-21 5:38 Evan Benn
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).