| Message ID | 20200403052900.258855-1-evanbenn@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add a watchdog driver that uses ARM Secure Monitor Calls. | expand |
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 >
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 >>
> 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.
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
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
> 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.)
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. > > . >
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.
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
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
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