* [PATCH 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls. @ 2020-02-14 6:26 Evan Benn 2020-02-14 6:26 ` [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible Evan Benn 2020-02-14 6:26 ` [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn 0 siblings, 2 replies; 24+ messages in thread From: Evan Benn @ 2020-02-14 6:26 UTC (permalink / raw) To: LKML Cc: jwerner, Evan Benn, Bjorn Andersson, Guenter Roeck, Mauro Carvalho Chehab, Rob Herring, Marcin Juszkiewicz, Catalin Marinas, Olof Johansson, Leonard Crestez, Jonathan Cameron, Dinh Nguyen, Shawn Guo, Greg Kroah-Hartman, devicetree, Will Deacon, linux-watchdog, Rob Herring, Wim Van Sebroeck, Clément Péron, linux-arm-kernel, David S. Miller, Mark Rutland, Anson Huang 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. Evan Benn (1): dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible Julius Werner (1): watchdog: Add new arm_smc_wdt watchdog driver .../bindings/watchdog/arm,smc-wdt.yaml | 30 +++ MAINTAINERS | 7 + arch/arm64/configs/defconfig | 1 + drivers/watchdog/Kconfig | 12 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/arm_smc_wdt.c | 191 ++++++++++++++++++ 6 files changed, 242 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/arm,smc-wdt.yaml create mode 100644 drivers/watchdog/arm_smc_wdt.c -- 2.25.0.265.gbab2e86ba0-goog ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-14 6:26 [PATCH 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Evan Benn @ 2020-02-14 6:26 ` Evan Benn 2020-02-19 22:30 ` Rob Herring 2020-02-14 6:26 ` [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn 1 sibling, 1 reply; 24+ messages in thread From: Evan Benn @ 2020-02-14 6:26 UTC (permalink / raw) To: LKML Cc: jwerner, Evan Benn, devicetree, Guenter Roeck, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Rob Herring, Greg Kroah-Hartman, Mark Rutland, 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> --- .../bindings/watchdog/arm,smc-wdt.yaml | 30 +++++++++++++++++++ MAINTAINERS | 6 ++++ 2 files changed, 36 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 000000000000..5170225b0c98 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/arm,smc-wdt.yaml @@ -0,0 +1,30 @@ +# 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: + - arm,smc-wdt + +required: + - compatible + +examples: + - | + watchdog { + compatible = "arm,smc-wdt"; + timeout-sec = <15>; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index e48ab79879ac..5c45536e1177 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1421,6 +1421,12 @@ S: Maintained F: Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt F: drivers/irqchip/irq-al-fic.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.25.0.265.gbab2e86ba0-goog ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-14 6:26 ` [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible Evan Benn @ 2020-02-19 22:30 ` Rob Herring 2020-02-19 23:04 ` Julius Werner 0 siblings, 1 reply; 24+ messages in thread From: Rob Herring @ 2020-02-19 22:30 UTC (permalink / raw) To: Evan Benn Cc: LKML, jwerner, devicetree, Guenter Roeck, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Greg Kroah-Hartman, Mark Rutland, linux-watchdog On Fri, Feb 14, 2020 at 05:26:36PM +1100, Evan Benn wrote: > 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> > --- > > .../bindings/watchdog/arm,smc-wdt.yaml | 30 +++++++++++++++++++ > MAINTAINERS | 6 ++++ > 2 files changed, 36 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 000000000000..5170225b0c98 > --- /dev/null > +++ b/Documentation/devicetree/bindings/watchdog/arm,smc-wdt.yaml > @@ -0,0 +1,30 @@ > +# 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 You are not the first 'watchdog in firmware accessed via an SMC call'. Is there some more detail about what implementation this is? Part of TF-A? Defined by some spec (I can dream)? > + > +allOf: > + - $ref: "watchdog.yaml#" > + > +maintainers: > + - Julius Werner <jwerner@chromium.org> > + > +properties: > + compatible: > + enum: > + - arm,smc-wdt > + > +required: > + - compatible > + > +examples: > + - | > + watchdog { I'd expect this to be a child of whatever firmware implements this function. > + compatible = "arm,smc-wdt"; > + timeout-sec = <15>; > + }; > + > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index e48ab79879ac..5c45536e1177 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1421,6 +1421,12 @@ S: Maintained > F: Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt > F: drivers/irqchip/irq-al-fic.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.25.0.265.gbab2e86ba0-goog > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-19 22:30 ` Rob Herring @ 2020-02-19 23:04 ` Julius Werner 2020-02-19 23:20 ` Guenter Roeck 0 siblings, 1 reply; 24+ messages in thread From: Julius Werner @ 2020-02-19 23:04 UTC (permalink / raw) To: Rob Herring Cc: Evan Benn, LKML, Julius Werner, devicetree, Guenter Roeck, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Greg Kroah-Hartman, Mark Rutland, linux-watchdog > You are not the first 'watchdog in firmware accessed via an SMC call'. > Is there some more detail about what implementation this is? Part of > TF-A? Defined by some spec (I can dream)? This is just some random implementation written by me because we needed one. I would like it to be the new generic implementation, but it sounds like people here prefer the naming to be MediaTek specific (at least for now). The other SMC watchdog we're aware of is imx_sc_wdt but unfortunately that seems to hardcode platform-specific details in the interface (at least in the pretimeout SMC) so we can't just expand that. With this driver I tried to directly wrap the kernel watchdog interface so it should be platform-agnostic and possible to expand this driver to other platforms later if desired. The SMC function ID would still always have to be platform-specific, unfortunately (but we could pass it in through the device tree), since the Arm SMC spec doesn't really leave any room for OS-generic SMCs like this. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-19 23:04 ` Julius Werner @ 2020-02-19 23:20 ` Guenter Roeck 2020-02-20 6:41 ` Evan Benn 0 siblings, 1 reply; 24+ messages in thread From: Guenter Roeck @ 2020-02-19 23:20 UTC (permalink / raw) To: Julius Werner Cc: Rob Herring, Evan Benn, LKML, devicetree, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Greg Kroah-Hartman, Mark Rutland, linux-watchdog On Wed, Feb 19, 2020 at 03:04:54PM -0800, Julius Werner wrote: > > You are not the first 'watchdog in firmware accessed via an SMC call'. > > Is there some more detail about what implementation this is? Part of > > TF-A? Defined by some spec (I can dream)? > > This is just some random implementation written by me because we > needed one. I would like it to be the new generic implementation, but > it sounds like people here prefer the naming to be MediaTek specific > (at least for now). The other SMC watchdog we're aware of is > imx_sc_wdt but unfortunately that seems to hardcode platform-specific There is one more pending, for Meson SMC. https://patchwork.kernel.org/project/linux-watchdog/list/?series=227733 Unfortunately it uses Meson firmware API functions, though it has pretty much the same functionality since those ultimately end up calling arm_smccc_smc(). Guenter > details in the interface (at least in the pretimeout SMC) so we can't > just expand that. With this driver I tried to directly wrap the kernel > watchdog interface so it should be platform-agnostic and possible to > expand this driver to other platforms later if desired. The SMC > function ID would still always have to be platform-specific, > unfortunately (but we could pass it in through the device tree), since > the Arm SMC spec doesn't really leave any room for OS-generic SMCs > like this. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-19 23:20 ` Guenter Roeck @ 2020-02-20 6:41 ` Evan Benn 2020-02-20 15:43 ` Guenter Roeck 2020-02-21 15:36 ` Xingyu Chen 0 siblings, 2 replies; 24+ messages in thread From: Evan Benn @ 2020-02-20 6:41 UTC (permalink / raw) To: xingyu.chen Cc: Julius Werner, Guenter Roeck, Rob Herring, LKML, devicetree, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Greg Kroah-Hartman, Mark Rutland, linux-watchdog Dear Xingyu, Could this driver also cover your usecase? I am not familiar with meson, but it seems like the meson calls could be replaced with arm_smccc calls. Then this driver will cover both chips. I am not sure if your firmware is upstream somewhere, but this might be adapted; https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405 Thanks On Thu, Feb 20, 2020 at 10:20 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On Wed, Feb 19, 2020 at 03:04:54PM -0800, Julius Werner wrote: > > > You are not the first 'watchdog in firmware accessed via an SMC call'. > > > Is there some more detail about what implementation this is? Part of > > > TF-A? Defined by some spec (I can dream)? > > > > This is just some random implementation written by me because we > > needed one. I would like it to be the new generic implementation, but > > it sounds like people here prefer the naming to be MediaTek specific > > (at least for now). The other SMC watchdog we're aware of is > > imx_sc_wdt but unfortunately that seems to hardcode platform-specific > > There is one more pending, for Meson SMC. > > https://patchwork.kernel.org/project/linux-watchdog/list/?series=227733 > > Unfortunately it uses Meson firmware API functions, though it has pretty > much the same functionality since those ultimately end up calling > arm_smccc_smc(). > > Guenter > > > details in the interface (at least in the pretimeout SMC) so we can't > > just expand that. With this driver I tried to directly wrap the kernel > > watchdog interface so it should be platform-agnostic and possible to > > expand this driver to other platforms later if desired. The SMC > > function ID would still always have to be platform-specific, > > unfortunately (but we could pass it in through the device tree), since > > the Arm SMC spec doesn't really leave any room for OS-generic SMCs > > like this. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-20 6:41 ` Evan Benn @ 2020-02-20 15:43 ` Guenter Roeck 2020-02-21 15:36 ` Xingyu Chen 1 sibling, 0 replies; 24+ messages in thread From: Guenter Roeck @ 2020-02-20 15:43 UTC (permalink / raw) To: Evan Benn Cc: xingyu.chen, Julius Werner, Rob Herring, LKML, devicetree, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Greg Kroah-Hartman, Mark Rutland, linux-watchdog On Thu, Feb 20, 2020 at 05:41:09PM +1100, Evan Benn wrote: > Dear Xingyu, > > Could this driver also cover your usecase? I am not familiar with > meson, but it seems like the meson calls could > be replaced with arm_smccc calls. Then this driver will cover both > chips. I am not sure if your firmware is upstream > somewhere, but this might be adapted; > https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405 > FWIW, the Meson driver has more functionality. Guenter > Thanks > > > On Thu, Feb 20, 2020 at 10:20 AM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On Wed, Feb 19, 2020 at 03:04:54PM -0800, Julius Werner wrote: > > > > You are not the first 'watchdog in firmware accessed via an SMC call'. > > > > Is there some more detail about what implementation this is? Part of > > > > TF-A? Defined by some spec (I can dream)? > > > > > > This is just some random implementation written by me because we > > > needed one. I would like it to be the new generic implementation, but > > > it sounds like people here prefer the naming to be MediaTek specific > > > (at least for now). The other SMC watchdog we're aware of is > > > imx_sc_wdt but unfortunately that seems to hardcode platform-specific > > > > There is one more pending, for Meson SMC. > > > > https://patchwork.kernel.org/project/linux-watchdog/list/?series=227733 > > > > Unfortunately it uses Meson firmware API functions, though it has pretty > > much the same functionality since those ultimately end up calling > > arm_smccc_smc(). > > > > Guenter > > > > > details in the interface (at least in the pretimeout SMC) so we can't > > > just expand that. With this driver I tried to directly wrap the kernel > > > watchdog interface so it should be platform-agnostic and possible to > > > expand this driver to other platforms later if desired. The SMC > > > function ID would still always have to be platform-specific, > > > unfortunately (but we could pass it in through the device tree), since > > > the Arm SMC spec doesn't really leave any room for OS-generic SMCs > > > like this. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-20 6:41 ` Evan Benn 2020-02-20 15:43 ` Guenter Roeck @ 2020-02-21 15:36 ` Xingyu Chen 2020-02-21 19:41 ` Julius Werner 1 sibling, 1 reply; 24+ messages in thread From: Xingyu Chen @ 2020-02-21 15:36 UTC (permalink / raw) To: Evan Benn Cc: Julius Werner, Guenter Roeck, Rob Herring, LKML, devicetree, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Greg Kroah-Hartman, Mark Rutland, linux-watchdog, Jianxin Pan, Yonghui Yu Hi, Evan Because the ATF does not define standard wdt index, each vendor defines its own index. So I don't think that the current driver[0] can fully cover my usecases. As discussed in your previous email, the meson wdt driver [1] can use the arm_smccc instead of meson_sm_call. [0]: https://patchwork.kernel.org/patch/11395579/ [1]: https://patchwork.kernel.org/patch/11331271/ Best Regards On 2020/2/20 14:41, Evan Benn wrote: > Dear Xingyu, > > Could this driver also cover your usecase? I am not familiar with > meson, but it seems like the meson calls could > be replaced with arm_smccc calls. Then this driver will cover both > chips. I am not sure if your firmware is upstream > somewhere, but this might be adapted; > https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405 > > Thanks > > > On Thu, Feb 20, 2020 at 10:20 AM Guenter Roeck <linux@roeck-us.net> wrote: >> On Wed, Feb 19, 2020 at 03:04:54PM -0800, Julius Werner wrote: >>>> You are not the first 'watchdog in firmware accessed via an SMC call'. >>>> Is there some more detail about what implementation this is? Part of >>>> TF-A? Defined by some spec (I can dream)? >>> This is just some random implementation written by me because we >>> needed one. I would like it to be the new generic implementation, but >>> it sounds like people here prefer the naming to be MediaTek specific >>> (at least for now). The other SMC watchdog we're aware of is >>> imx_sc_wdt but unfortunately that seems to hardcode platform-specific >> There is one more pending, for Meson SMC. >> >> https://patchwork.kernel.org/project/linux-watchdog/list/?series=227733 >> >> Unfortunately it uses Meson firmware API functions, though it has pretty >> much the same functionality since those ultimately end up calling >> arm_smccc_smc(). >> >> Guenter >> >>> details in the interface (at least in the pretimeout SMC) so we can't >>> just expand that. With this driver I tried to directly wrap the kernel >>> watchdog interface so it should be platform-agnostic and possible to >>> expand this driver to other platforms later if desired. The SMC >>> function ID would still always have to be platform-specific, >>> unfortunately (but we could pass it in through the device tree), since >>> the Arm SMC spec doesn't really leave any room for OS-generic SMCs >>> like this. > . ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-21 15:36 ` Xingyu Chen @ 2020-02-21 19:41 ` Julius Werner 2020-02-21 20:46 ` Guenter Roeck 2020-02-22 4:01 ` Xingyu Chen 0 siblings, 2 replies; 24+ messages in thread From: Julius Werner @ 2020-02-21 19:41 UTC (permalink / raw) To: Xingyu Chen Cc: Evan Benn, Julius Werner, Guenter Roeck, Rob Herring, LKML, devicetree, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Greg Kroah-Hartman, Mark Rutland, linux-watchdog, Jianxin Pan, Yonghui Yu > Because the ATF does not define standard wdt index, each vendor defines > its own index. > So I don't think that the current driver[0] can fully cover my usecases. I think the best way to solve this would be to put the SMC function ID as another field into the device tree, so that multiple vendors could share the same driver even if their firmware interface uses a different SMC. But they still have to implement the same API for that SMC, of course, not sure if the Meson driver is suitable for that (but if it is then I think merging those drivers would be a good idea). ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-21 19:41 ` Julius Werner @ 2020-02-21 20:46 ` Guenter Roeck 2020-02-22 4:01 ` Xingyu Chen 1 sibling, 0 replies; 24+ messages in thread From: Guenter Roeck @ 2020-02-21 20:46 UTC (permalink / raw) To: Julius Werner Cc: Xingyu Chen, Evan Benn, Rob Herring, LKML, devicetree, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Greg Kroah-Hartman, Mark Rutland, linux-watchdog, Jianxin Pan, Yonghui Yu On Fri, Feb 21, 2020 at 11:41:47AM -0800, Julius Werner wrote: > > Because the ATF does not define standard wdt index, each vendor defines > > its own index. > > So I don't think that the current driver[0] can fully cover my usecases. > > I think the best way to solve this would be to put the SMC function ID > as another field into the device tree, so that multiple vendors could > share the same driver even if their firmware interface uses a > different SMC. But they still have to implement the same API for that > SMC, of course, not sure if the Meson driver is suitable for that (but > if it is then I think merging those drivers would be a good idea). More common would be to have a single driver and multiple compatible strings. The driver would then pick the SMC function ID from the compatible string. After all, we'd not only need the SMC function ID; parameters are also different. Guenter ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-21 19:41 ` Julius Werner 2020-02-21 20:46 ` Guenter Roeck @ 2020-02-22 4:01 ` Xingyu Chen 2020-02-24 1:10 ` Evan Benn 2020-02-25 1:23 ` Julius Werner 1 sibling, 2 replies; 24+ messages in thread From: Xingyu Chen @ 2020-02-22 4:01 UTC (permalink / raw) To: Julius Werner Cc: Evan Benn, Guenter Roeck, Rob Herring, LKML, devicetree, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Greg Kroah-Hartman, Mark Rutland, linux-watchdog, Jianxin Pan, Yonghui Yu, Xingyu Chen Hi, Julius On 2020/2/22 3:41, Julius Werner wrote: >> Because the ATF does not define standard wdt index, each vendor defines >> its own index. >> So I don't think that the current driver[0] can fully cover my usecases. > I think the best way to solve this would be to put the SMC function ID > as another field into the device tree, so that multiple vendors could > share the same driver even if their firmware interface uses a > different SMC. But they still have to implement the same API for that > SMC, of course, not sure if the Meson driver is suitable for that (but > if it is then I think merging those drivers would be a good idea). The SMC function ID may be solved by the DTS, but the wdt indexs(Eg: SMCWD_INFO) are also different for each vendor. The imx_sc_wdt.c is also use the SMC to operate the WDT, but the wdt indexs(Eg: IMX_SIP_TIMER_START_WDOG) are different from ours. IMO, If the ATF can implement a common hal interface and index for watchdog, then writing a common smc wdt driver will be easier to compatible with all vendors. Best Regards > > . ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-22 4:01 ` Xingyu Chen @ 2020-02-24 1:10 ` Evan Benn 2020-02-25 1:23 ` Julius Werner 1 sibling, 0 replies; 24+ messages in thread From: Evan Benn @ 2020-02-24 1:10 UTC (permalink / raw) To: Xingyu Chen Cc: Julius Werner, Guenter Roeck, Rob Herring, LKML, devicetree, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Greg Kroah-Hartman, Mark Rutland, linux-watchdog, Jianxin Pan, Yonghui Yu Hello, I think the intention is that this driver talks to a 'standard' arm smc firmware watchdog call: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405 Each device could re-implement that ATF driver to talk to the specific hardware, and could perhaps use a custom SMCWD_FUNC_ID, defined in the dts. The goal was to provide an ATF patch and linux driver patch that would be generic. But the above ATF patch is only for mt8173. Right now it just specifies an interface. It has less functionality than your meson driver Xingyu. If it is not suitable, that is fine. The above ATF patch is deployed on oak, elm, and hana mt8173 chromebook devices, this driver is intended to support those devices. Evan On Sat, Feb 22, 2020 at 3:01 PM Xingyu Chen <xingyu.chen@amlogic.com> wrote: > > Hi, Julius > > On 2020/2/22 3:41, Julius Werner wrote: > >> Because the ATF does not define standard wdt index, each vendor defines > >> its own index. > >> So I don't think that the current driver[0] can fully cover my usecases. > > I think the best way to solve this would be to put the SMC function ID > > as another field into the device tree, so that multiple vendors could > > share the same driver even if their firmware interface uses a > > different SMC. But they still have to implement the same API for that > > SMC, of course, not sure if the Meson driver is suitable for that (but > > if it is then I think merging those drivers would be a good idea). > The SMC function ID may be solved by the DTS, but the wdt indexs(Eg: > SMCWD_INFO) are also different > for each vendor. The imx_sc_wdt.c is also use the SMC to operate the > WDT, but the wdt indexs(Eg: IMX_SIP_TIMER_START_WDOG) > are different from ours. IMO, If the ATF can implement a common hal > interface and index for watchdog, then writing a > common smc wdt driver will be easier to compatible with all vendors. > > Best Regards > > > > . ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-22 4:01 ` Xingyu Chen 2020-02-24 1:10 ` Evan Benn @ 2020-02-25 1:23 ` Julius Werner 2020-02-25 7:44 ` Xingyu Chen 1 sibling, 1 reply; 24+ messages in thread From: Julius Werner @ 2020-02-25 1:23 UTC (permalink / raw) To: Xingyu Chen Cc: Julius Werner, Evan Benn, Guenter Roeck, Rob Herring, LKML, devicetree, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Greg Kroah-Hartman, Mark Rutland, linux-watchdog, Jianxin Pan, Yonghui Yu > The SMC function ID may be solved by the DTS, but the wdt indexs(Eg: > SMCWD_INFO) are also different > for each vendor. The imx_sc_wdt.c is also use the SMC to operate the > WDT, but the wdt indexs(Eg: IMX_SIP_TIMER_START_WDOG) > are different from ours. IMO, If the ATF can implement a common hal > interface and index for watchdog, then writing a > common smc wdt driver will be easier to compatible with all vendors. The MediaTek driver is still in flux (e.g. still being reviewed in Trusted Firmware here: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405), we can still change it. So if we can now decide on making this a "standard" driver, we can change the MediaTek interface to match IMX and standardize on that. (There are existing Chromebooks shipped with a different interface, but we could handle those separately with downstream patches. I think having a unified interface that will prevent this problem in the future would be worth some extra complication right now.) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-25 1:23 ` Julius Werner @ 2020-02-25 7:44 ` Xingyu Chen 2020-03-10 1:00 ` Evan Benn 0 siblings, 1 reply; 24+ messages in thread From: Xingyu Chen @ 2020-02-25 7:44 UTC (permalink / raw) To: Julius Werner Cc: Evan Benn, Guenter Roeck, Rob Herring, LKML, devicetree, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Greg Kroah-Hartman, Mark Rutland, linux-watchdog, Jianxin Pan, Yonghui Yu, open list:ARM/Amlogic Meson... Hi, Julius On 2020/2/25 9:23, Julius Werner wrote: >> The SMC function ID may be solved by the DTS, but the wdt indexs(Eg: >> SMCWD_INFO) are also different >> for each vendor. The imx_sc_wdt.c is also use the SMC to operate the >> WDT, but the wdt indexs(Eg: IMX_SIP_TIMER_START_WDOG) >> are different from ours. IMO, If the ATF can implement a common hal >> interface and index for watchdog, then writing a >> common smc wdt driver will be easier to compatible with all vendors. > The MediaTek driver is still in flux (e.g. still being reviewed in > Trusted Firmware here: > https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405), > we can still change it. So if we can now decide on making this a > "standard" driver, we can change the MediaTek interface to match IMX > and standardize on that. (There are existing Chromebooks shipped with > a different interface, but we could handle those separately with > downstream patches. I think having a unified interface that will > prevent this problem in the future would be worth some extra > complication right now.) If the ATF provides a common watchdog hal interface and index, I am happy to match the generic sec wdt driver. Compared to the current MTK wdt index [0], the following indexes need to be supported for meson wdt [1]. - *_INIT. - *_GETTIMEOUT. - *_RESETNOW. It is used to reset the system right now, similar to your SOFT RESET. For another platform-specific parameter "SMC function ID", the generic sec wdt driver can get it from the dts, but if the driver want to compatible with more vendors in the future, maybe we should consider Guenter's suggestion at [2] [0]: https://patchwork.kernel.org/patch/11395579/ [1]: https://patchwork.kernel.org/patch/11331271/ [2]: https://lore.kernel.org/linux-watchdog/20200220155159.GB29658@roeck-us.net/T/#md00328548222965054cd19ec7dda074f8fc09fe2 Best Regards > . ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-02-25 7:44 ` Xingyu Chen @ 2020-03-10 1:00 ` Evan Benn [not found] ` <ad28aa47-0490-937f-898f-0e4695ef6ec6@amlogic.com> 0 siblings, 1 reply; 24+ messages in thread From: Evan Benn @ 2020-03-10 1:00 UTC (permalink / raw) To: Xingyu Chen Cc: Julius Werner, Guenter Roeck, Rob Herring, LKML, devicetree, David S. Miller, Jonathan Cameron, Mauro Carvalho Chehab, Wim Van Sebroeck, Greg Kroah-Hartman, Mark Rutland, LINUX-WATCHDOG, Jianxin Pan, Yonghui Yu, open list:ARM/Amlogic Meson... Hi Xingyu, I am trying to establish some clarity about what to do here. The trusted firmware review has now been accepted https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405. I could try to add your mentioned extra operation indexes to the ATF watchdog, to try to establish a standard ATF smc watchdog interface. Hypothetically then your linux driver could connect to any of the ATF watchdogs, apart from the meson indirection layer. I do not quite understand the meson layer to be honest, would we run the meson layer on non-amlogic SOCs? It looks feasible to strip the meson part from your driver so that it works on more socs, please correct me if I am wrong. Alternatively we could also add these extra operation indexes to this linux driver. Unfortunately I would not have a way to test that. Thanks Evan On Tue, Feb 25, 2020 at 6:43 PM Xingyu Chen <xingyu.chen@amlogic.com> wrote: > > Hi, Julius > > On 2020/2/25 9:23, Julius Werner wrote: > >> The SMC function ID may be solved by the DTS, but the wdt indexs(Eg: > >> SMCWD_INFO) are also different > >> for each vendor. The imx_sc_wdt.c is also use the SMC to operate the > >> WDT, but the wdt indexs(Eg: IMX_SIP_TIMER_START_WDOG) > >> are different from ours. IMO, If the ATF can implement a common hal > >> interface and index for watchdog, then writing a > >> common smc wdt driver will be easier to compatible with all vendors. > > The MediaTek driver is still in flux (e.g. still being reviewed in > > Trusted Firmware here: > > https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405), > > we can still change it. So if we can now decide on making this a > > "standard" driver, we can change the MediaTek interface to match IMX > > and standardize on that. (There are existing Chromebooks shipped with > > a different interface, but we could handle those separately with > > downstream patches. I think having a unified interface that will > > prevent this problem in the future would be worth some extra > > complication right now.) > If the ATF provides a common watchdog hal interface and index, I am > happy to match > the generic sec wdt driver. Compared to the current MTK wdt index [0], > the following > indexes need to be supported for meson wdt [1]. > - *_INIT. > - *_GETTIMEOUT. > - *_RESETNOW. It is used to reset the system right now, similar to your > SOFT RESET. > > For another platform-specific parameter "SMC function ID", the generic > sec wdt driver can get it from the dts, but if > the driver want to compatible with more vendors in the future, maybe we > should consider Guenter's suggestion at [2] > > [0]: https://patchwork.kernel.org/patch/11395579/ > [1]: https://patchwork.kernel.org/patch/11331271/ > [2]: > https://lore.kernel.org/linux-watchdog/20200220155159.GB29658@roeck-us.net/T/#md00328548222965054cd19ec7dda074f8fc09fe2 > > Best Regards > > . ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <ad28aa47-0490-937f-898f-0e4695ef6ec6@amlogic.com>]
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible [not found] ` <ad28aa47-0490-937f-898f-0e4695ef6ec6@amlogic.com> @ 2020-03-11 19:24 ` Julius Werner 2020-03-13 16:13 ` Xingyu Chen 0 siblings, 1 reply; 24+ messages in thread From: Julius Werner @ 2020-03-11 19:24 UTC (permalink / raw) To: Xingyu Chen Cc: Evan Benn, Mark Rutland, devicetree, LINUX-WATCHDOG, Jianxin Pan, Rob Herring, Greg Kroah-Hartman, LKML, Yonghui Yu, Jonathan Cameron, Mauro Carvalho Chehab, Julius Werner, open list:ARM/Amlogic Meson..., Wim Van Sebroeck, David S. Miller, Guenter Roeck > - *_INIT and *GETTIMEOUT. Although your driver does not need them, could you take them as options in your driver ? The driver already has SMCWD_INFO which is used during probe to retrieve the minimum and maximum timeout values supported by the hardware at probe time. Maybe it would make sense to rename that to INIT (which would still return those values, but can also do whatever initialization needs to be done in TF)? GETTIMELEFT I agree we can implement optionally, and other platforms would just return a PSCI_RET_NOT_SUPPORTED for that. > - *_RESETNOW. It is used to reset the system right now, similar to your SOFT RESET. could you reserve an operation index in ATF ? Just curious, why do you need this? Shouldn't you use the PSCI standard SYSTEM_RESET SMC for that? (If you want to control exactly how the platform is reset, you could also use SYSTEM_RESET2 with a vendor-defined reset_type.) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible 2020-03-11 19:24 ` Julius Werner @ 2020-03-13 16:13 ` Xingyu Chen 0 siblings, 0 replies; 24+ messages in thread From: Xingyu Chen @ 2020-03-13 16:13 UTC (permalink / raw) To: Julius Werner Cc: Evan Benn, Mark Rutland, devicetree, LINUX-WATCHDOG, Jianxin Pan, Rob Herring, Greg Kroah-Hartman, LKML, Yonghui Yu, Jonathan Cameron, Mauro Carvalho Chehab, open list:ARM/Amlogic Meson..., Wim Van Sebroeck, David S. Miller, Guenter Roeck Hi, Julius On 2020/3/12 3:24, Julius Werner wrote: >> - *_INIT and *GETTIMEOUT. Although your driver does not need them, could you take them as options in your driver ? > The driver already has SMCWD_INFO which is used during probe to > retrieve the minimum and maximum timeout values supported by the > hardware at probe time. Maybe it would make sense to rename that to > INIT (which would still return those values, but can also do whatever > initialization needs to be done in TF)? Yes,INIT would make sense for me. > GETTIMELEFT I agree we can > implement optionally, and other platforms would just return a > PSCI_RET_NOT_SUPPORTED for that. > >> - *_RESETNOW. It is used to reset the system right now, similar to your SOFT RESET. could you reserve an operation index in ATF ? > Just curious, why do you need this? Shouldn't you use the PSCI > standard SYSTEM_RESET SMC for that? (If you want to control exactly > how the platform is reset, you could also use SYSTEM_RESET2 with a > vendor-defined reset_type.) I just wanted it to be compatible with other OS,and I think it over, maybe I can also use the PSCI interface to execuate the system reset on the other OS. Anyway, please ignore this request. Thanks. > > . ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-02-14 6:26 [PATCH 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Evan Benn 2020-02-14 6:26 ` [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible Evan Benn @ 2020-02-14 6:26 ` Evan Benn 2020-02-14 9:35 ` Enric Balletbo Serra 2020-02-14 14:10 ` Guenter Roeck 1 sibling, 2 replies; 24+ messages in thread From: Evan Benn @ 2020-02-14 6:26 UTC (permalink / raw) To: LKML Cc: jwerner, Evan Benn, Bjorn Andersson, Guenter Roeck, Mauro Carvalho Chehab, Marcin Juszkiewicz, Catalin Marinas, Olof Johansson, Leonard Crestez, Jonathan Cameron, Dinh Nguyen, linux-arm-kernel, Greg Kroah-Hartman, Will Deacon, linux-watchdog, Rob Herring, Wim Van Sebroeck, Clément Péron, Shawn Guo, David S. Miller, Anson Huang From: Julius Werner <jwerner@chromium.org> This patch adds a stub watchdog driver that can be used on ARM systems with a Secure Monitor firmware to forward watchdog operations to 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. Signed-off-by: Julius Werner <jwerner@chromium.org> Signed-off-by: Evan Benn <evanbenn@chromium.org> --- MAINTAINERS | 1 + arch/arm64/configs/defconfig | 1 + drivers/watchdog/Kconfig | 12 +++ drivers/watchdog/Makefile | 1 + drivers/watchdog/arm_smc_wdt.c | 191 +++++++++++++++++++++++++++++++++ 5 files changed, 206 insertions(+) create mode 100644 drivers/watchdog/arm_smc_wdt.c diff --git a/MAINTAINERS b/MAINTAINERS index 5c45536e1177..71df3c110fdb 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1426,6 +1426,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 b2f667307f82..8527db9e92a6 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -451,6 +451,7 @@ CONFIG_QCOM_TSENS=y CONFIG_UNIPHIER_THERMAL=y CONFIG_WATCHDOG=y CONFIG_ARM_SP805_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 cec868f8db3f..0f7f93342051 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -857,6 +857,18 @@ 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 + 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 2ee352bf3372..a1e6d83a7659 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -92,6 +92,7 @@ obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o obj-$(CONFIG_RTD119X_WATCHDOG) += rtd119x_wdt.o obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o +obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o # X86 (i386 + ia64 + x86_64) Architecture diff --git a/drivers/watchdog/arm_smc_wdt.c b/drivers/watchdog/arm_smc_wdt.c new file mode 100644 index 000000000000..58e7294136ef --- /dev/null +++ b/drivers/watchdog/arm_smc_wdt.c @@ -0,0 +1,191 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * ARM Secure Monitor Call watchdog driver + * + * Copyright 2018 The Chromium OS Authors. All rights reserved. + * + * Julius Werner <jwerner@chromium.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * 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 SMCWD_FUNC_ID 0x82003d06 + +enum smcwd_call { + SMCWD_INFO = 0, + SMCWD_SET_TIMEOUT = 1, + SMCWD_ENABLE = 2, + SMCWD_PET = 3, +}; + +static bool nowayout = WATCHDOG_NOWAYOUT; +static unsigned int timeout; + +static int smcwd_call(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(SMCWD_FUNC_ID, call, arg, 0, 0, 0, 0, 0, res); + + if ((int)res->a0 == PSCI_RET_NOT_SUPPORTED) + return -ENOTSUPP; + if ((int)res->a0 == PSCI_RET_INVALID_PARAMS) + return -EINVAL; + if ((int)res->a0 < 0) + return -EIO; + return res->a0; +} + +static int smcwd_ping(struct watchdog_device *wdd) +{ + return smcwd_call(SMCWD_PET, 0, NULL); +} + +static int smcwd_set_timeout(struct watchdog_device *wdd, + unsigned int timeout) +{ + int res; + + res = smcwd_call(SMCWD_SET_TIMEOUT, timeout, NULL); + if (!res) + wdd->timeout = timeout; + return res; +} + +static int smcwd_stop(struct watchdog_device *wdd) +{ + return smcwd_call(SMCWD_ENABLE, 0, NULL); +} + +static int smcwd_start(struct watchdog_device *wdd) +{ + return smcwd_call(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 = { + .owner = THIS_MODULE, + .start = smcwd_start, + .stop = smcwd_stop, + .ping = smcwd_ping, + .set_timeout = smcwd_set_timeout, +}; + +static int smcwd_probe(struct platform_device *pdev) +{ + struct watchdog_device *wdd; + int err; + struct arm_smccc_res res; + + err = smcwd_call(SMCWD_INFO, 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); + + wdd->info = &smcwd_info; + wdd->ops = &smcwd_ops; + wdd->timeout = res.a2; + wdd->max_timeout = res.a2; + wdd->min_timeout = res.a1; + wdd->parent = &pdev->dev; + + watchdog_set_nowayout(wdd, nowayout); + watchdog_init_timeout(wdd, timeout, &pdev->dev); + err = smcwd_set_timeout(wdd, wdd->timeout); + if (err) + return err; + + err = watchdog_register_device(wdd); + if (err) + return err; + + dev_info(&pdev->dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n", + wdd->timeout, nowayout); + + return 0; +} + +static void smcwd_shutdown(struct platform_device *pdev) +{ + struct watchdog_device *wdd = platform_get_drvdata(pdev); + + if (watchdog_active(wdd)) + smcwd_stop(wdd); +} + +static int smcwd_remove(struct platform_device *pdev) +{ + struct watchdog_device *wdd = platform_get_drvdata(pdev); + + watchdog_unregister_device(wdd); + + return 0; +} + +static const struct of_device_id smcwd_dt_ids[] = { + { .compatible = "arm,smc-wdt" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, smcwd_dt_ids); + +static struct platform_driver smcwd_driver = { + .probe = smcwd_probe, + .remove = smcwd_remove, + .shutdown = smcwd_shutdown, + .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.25.0.265.gbab2e86ba0-goog ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-02-14 6:26 ` [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn @ 2020-02-14 9:35 ` Enric Balletbo Serra 2020-02-14 14:10 ` Guenter Roeck 1 sibling, 0 replies; 24+ messages in thread From: Enric Balletbo Serra @ 2020-02-14 9:35 UTC (permalink / raw) To: Evan Benn Cc: LKML, jwerner, Bjorn Andersson, Guenter Roeck, Mauro Carvalho Chehab, Marcin Juszkiewicz, Catalin Marinas, Olof Johansson, Leonard Crestez, Jonathan Cameron, Dinh Nguyen, Linux ARM, Greg Kroah-Hartman, Will Deacon, linux-watchdog, Rob Herring, Wim Van Sebroeck, Clément Péron, Shawn Guo, David S. Miller, Anson Huang Hi Evan and Julius, Many thanks for sending this upstream, I have some trivial comments, especially the license one, other than that, the driver looks good to me. Missatge de Evan Benn <evanbenn@chromium.org> del dia dv., 14 de febr. 2020 a les 7:28: > > From: Julius Werner <jwerner@chromium.org> > > This patch adds a stub watchdog driver that can be used on ARM systems > with a Secure Monitor firmware to forward watchdog operations to > 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. > > Signed-off-by: Julius Werner <jwerner@chromium.org> > Signed-off-by: Evan Benn <evanbenn@chromium.org> > --- > > MAINTAINERS | 1 + > arch/arm64/configs/defconfig | 1 + > drivers/watchdog/Kconfig | 12 +++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/arm_smc_wdt.c | 191 +++++++++++++++++++++++++++++++++ > 5 files changed, 206 insertions(+) > create mode 100644 drivers/watchdog/arm_smc_wdt.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 5c45536e1177..71df3c110fdb 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1426,6 +1426,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 b2f667307f82..8527db9e92a6 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -451,6 +451,7 @@ CONFIG_QCOM_TSENS=y > CONFIG_UNIPHIER_THERMAL=y > CONFIG_WATCHDOG=y > CONFIG_ARM_SP805_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 cec868f8db3f..0f7f93342051 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -857,6 +857,18 @@ 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 Looks like this driver is OF only, so add a dependency on CONFIG_OF > + 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 2ee352bf3372..a1e6d83a7659 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -92,6 +92,7 @@ obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o > obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o > obj-$(CONFIG_RTD119X_WATCHDOG) += rtd119x_wdt.o > obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o > +obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o > obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o > > # X86 (i386 + ia64 + x86_64) Architecture > diff --git a/drivers/watchdog/arm_smc_wdt.c b/drivers/watchdog/arm_smc_wdt.c > new file mode 100644 > index 000000000000..58e7294136ef > --- /dev/null > +++ b/drivers/watchdog/arm_smc_wdt.c > @@ -0,0 +1,191 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * ARM Secure Monitor Call watchdog driver > + * > + * Copyright 2018 The Chromium OS Authors. All rights reserved. > + * > + * Julius Werner <jwerner@chromium.org> > + * When adding new files to the kernel, use the regular Google copyright header to them. The main reason for this is that there‘s no concept of “The Chromium OS Authors” outside of Chromium OS project, since it refers to the AUTHORS file that isn’t bundled with the kernel. [1] In this case, it should be something like this: /* SPDX-License-Identifier: GPL-2.0-only */ /* * ARM Secure Monitor Call watchdog driver * * Copyright 2020 Google LLC. */ See: https://chromium.googlesource.com/chromiumos/docs/+/master/kernel_faq.md > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * Based on mtk_wdt.c Remove the license boilerplate, it is implicit in the SPDX tag. The reasoning is to avoid mismatches like what happens here. You are licensing the file as GPL-2.0-only but the boilerplate is GPL-2.0-or-later. > + */ > + > +#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 SMCWD_FUNC_ID 0x82003d06 > + > +enum smcwd_call { > + SMCWD_INFO = 0, > + SMCWD_SET_TIMEOUT = 1, > + SMCWD_ENABLE = 2, > + SMCWD_PET = 3, > +}; > + > +static bool nowayout = WATCHDOG_NOWAYOUT; > +static unsigned int timeout; > + > +static int smcwd_call(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(SMCWD_FUNC_ID, call, arg, 0, 0, 0, 0, 0, res); > + > + if ((int)res->a0 == PSCI_RET_NOT_SUPPORTED) Is this cast really needed? > + return -ENOTSUPP; > + if ((int)res->a0 == PSCI_RET_INVALID_PARAMS) ditto > + return -EINVAL; > + if ((int)res->a0 < 0) ditto > + return -EIO; > + return res->a0; > +} > + > +static int smcwd_ping(struct watchdog_device *wdd) > +{ > + return smcwd_call(SMCWD_PET, 0, NULL); > +} > + > +static int smcwd_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) nit: Alignment should match open parenthesis ( if you want to follow what checkpatch says with the --strict option ) > +{ > + int res; > + > + res = smcwd_call(SMCWD_SET_TIMEOUT, timeout, NULL); > + if (!res) > + wdd->timeout = timeout; > + return res; > +} > + > +static int smcwd_stop(struct watchdog_device *wdd) > +{ > + return smcwd_call(SMCWD_ENABLE, 0, NULL); > +} > + > +static int smcwd_start(struct watchdog_device *wdd) > +{ > + return smcwd_call(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 = { > + .owner = THIS_MODULE, > + .start = smcwd_start, > + .stop = smcwd_stop, > + .ping = smcwd_ping, > + .set_timeout = smcwd_set_timeout, > +}; > + > +static int smcwd_probe(struct platform_device *pdev) > +{ > + struct watchdog_device *wdd; > + int err; > + struct arm_smccc_res res; > + > + err = smcwd_call(SMCWD_INFO, 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); > + > + wdd->info = &smcwd_info; > + wdd->ops = &smcwd_ops; > + wdd->timeout = res.a2; > + wdd->max_timeout = res.a2; > + wdd->min_timeout = res.a1; > + wdd->parent = &pdev->dev; > + > + watchdog_set_nowayout(wdd, nowayout); > + watchdog_init_timeout(wdd, timeout, &pdev->dev); > + err = smcwd_set_timeout(wdd, wdd->timeout); > + if (err) > + return err; > + > + err = watchdog_register_device(wdd); > + if (err) > + return err; > + > + dev_info(&pdev->dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n", > + wdd->timeout, nowayout); > + > + return 0; > +} > + > +static void smcwd_shutdown(struct platform_device *pdev) > +{ > + struct watchdog_device *wdd = platform_get_drvdata(pdev); > + > + if (watchdog_active(wdd)) > + smcwd_stop(wdd); > +} > + > +static int smcwd_remove(struct platform_device *pdev) > +{ > + struct watchdog_device *wdd = platform_get_drvdata(pdev); > + > + watchdog_unregister_device(wdd); > + > + return 0; > +} > + > +static const struct of_device_id smcwd_dt_ids[] = { > + { .compatible = "arm,smc-wdt" }, > + { /* sentinel */ } nit: not sure about this subsystem, but usually the /* sentinel */ word is removed because is really trivial what is { } at the end of struct. > +}; > +MODULE_DEVICE_TABLE(of, smcwd_dt_ids); > + > +static struct platform_driver smcwd_driver = { > + .probe = smcwd_probe, > + .remove = smcwd_remove, > + .shutdown = smcwd_shutdown, > + .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.25.0.265.gbab2e86ba0-goog > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-02-14 6:26 ` [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn 2020-02-14 9:35 ` Enric Balletbo Serra @ 2020-02-14 14:10 ` Guenter Roeck 2020-02-14 21:32 ` Julius Werner 2020-02-20 6:50 ` Evan Benn 1 sibling, 2 replies; 24+ messages in thread From: Guenter Roeck @ 2020-02-14 14:10 UTC (permalink / raw) To: Evan Benn, LKML Cc: jwerner, Bjorn Andersson, Mauro Carvalho Chehab, Marcin Juszkiewicz, Catalin Marinas, Olof Johansson, Leonard Crestez, Jonathan Cameron, Dinh Nguyen, linux-arm-kernel, Greg Kroah-Hartman, Will Deacon, linux-watchdog, Rob Herring, Wim Van Sebroeck, Clément Péron, Shawn Guo, David S. Miller, Anson Huang On 2/13/20 10:26 PM, Evan Benn wrote: > From: Julius Werner <jwerner@chromium.org> > > This patch adds a stub watchdog driver that can be used on ARM systems From kernel perspective, this is not a "stub" driver. A stub driver is one that doesn't do anything. > with a Secure Monitor firmware to forward watchdog operations to > 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. > As written, one would assume this to work on all systems implementing ARM secure firmware, which is not the case. Please select a different name, and provide information about the systems where this is actually supported. If it happens to be standardized, we will need a reference to the standard supported. This needs to distinguish from IMX_SC_WDT, which also supports a secure monitor based watchdog (but doesn't claim to be generic). > Signed-off-by: Julius Werner <jwerner@chromium.org> > Signed-off-by: Evan Benn <evanbenn@chromium.org> I won't comment on items already covered by Enric. Please address those as well. > --- > > MAINTAINERS | 1 + > arch/arm64/configs/defconfig | 1 + > drivers/watchdog/Kconfig | 12 +++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/arm_smc_wdt.c | 191 +++++++++++++++++++++++++++++++++ > 5 files changed, 206 insertions(+) > create mode 100644 drivers/watchdog/arm_smc_wdt.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 5c45536e1177..71df3c110fdb 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1426,6 +1426,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 b2f667307f82..8527db9e92a6 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -451,6 +451,7 @@ CONFIG_QCOM_TSENS=y > CONFIG_UNIPHIER_THERMAL=y > CONFIG_WATCHDOG=y > CONFIG_ARM_SP805_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 cec868f8db3f..0f7f93342051 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -857,6 +857,18 @@ 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 This also 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. > + > + Extra empty line. > config LPC18XX_WATCHDOG > tristate "LPC18xx/43xx Watchdog" > depends on ARCH_LPC18XX || COMPILE_TEST > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 2ee352bf3372..a1e6d83a7659 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -92,6 +92,7 @@ obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o > obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o > obj-$(CONFIG_RTD119X_WATCHDOG) += rtd119x_wdt.o > obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o > +obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o > obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o > > # X86 (i386 + ia64 + x86_64) Architecture > diff --git a/drivers/watchdog/arm_smc_wdt.c b/drivers/watchdog/arm_smc_wdt.c > new file mode 100644 > index 000000000000..58e7294136ef > --- /dev/null > +++ b/drivers/watchdog/arm_smc_wdt.c > @@ -0,0 +1,191 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * ARM Secure Monitor Call watchdog driver > + * > + * Copyright 2018 The Chromium OS Authors. All rights reserved. > + * > + * Julius Werner <jwerner@chromium.org> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * 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 SMCWD_FUNC_ID 0x82003d06 > + > +enum smcwd_call { > + SMCWD_INFO = 0, > + SMCWD_SET_TIMEOUT = 1, > + SMCWD_ENABLE = 2, > + SMCWD_PET = 3, > +}; > + > +static bool nowayout = WATCHDOG_NOWAYOUT; > +static unsigned int timeout; > + > +static int smcwd_call(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(SMCWD_FUNC_ID, call, arg, 0, 0, 0, 0, 0, res); > + > + if ((int)res->a0 == PSCI_RET_NOT_SUPPORTED) > + return -ENOTSUPP; -ENODEV would be better here. > + if ((int)res->a0 == PSCI_RET_INVALID_PARAMS) > + return -EINVAL; > + if ((int)res->a0 < 0) > + return -EIO; Yes, those typecasts are indeed unnecessary. > + return res->a0; > +} > + > +static int smcwd_ping(struct watchdog_device *wdd) > +{ > + return smcwd_call(SMCWD_PET, 0, NULL); > +} > + > +static int smcwd_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + int res; > + > + res = smcwd_call(SMCWD_SET_TIMEOUT, timeout, NULL); > + if (!res) > + wdd->timeout = timeout; > + return res; > +} > + > +static int smcwd_stop(struct watchdog_device *wdd) > +{ > + return smcwd_call(SMCWD_ENABLE, 0, NULL); > +} > + > +static int smcwd_start(struct watchdog_device *wdd) > +{ > + return smcwd_call(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 = { > + .owner = THIS_MODULE, Not necessary, and will result in a static analyzer warning. > + .start = smcwd_start, > + .stop = smcwd_stop, > + .ping = smcwd_ping, > + .set_timeout = smcwd_set_timeout, > +}; > + > +static int smcwd_probe(struct platform_device *pdev) > +{ > + struct watchdog_device *wdd; > + int err; > + struct arm_smccc_res res; > + > + err = smcwd_call(SMCWD_INFO, 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); > + > + wdd->info = &smcwd_info; > + wdd->ops = &smcwd_ops; > + wdd->timeout = res.a2; > + wdd->max_timeout = res.a2; > + wdd->min_timeout = res.a1; > + wdd->parent = &pdev->dev; > + > + watchdog_set_nowayout(wdd, nowayout); > + watchdog_init_timeout(wdd, timeout, &pdev->dev); > + err = smcwd_set_timeout(wdd, wdd->timeout); > + if (err) > + return err; > + > + err = watchdog_register_device(wdd); devm_watchdog_register_device() should work here. > + if (err) > + return err; > + > + dev_info(&pdev->dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n", The watchdog is not enabled here, it is only registered. > + wdd->timeout, nowayout); > + > + return 0; > +} > + > +static void smcwd_shutdown(struct platform_device *pdev) > +{ > + struct watchdog_device *wdd = platform_get_drvdata(pdev); > + > + if (watchdog_active(wdd)) > + smcwd_stop(wdd); > +} > + Please use watchdog_stop_on_reboot(). > +static int smcwd_remove(struct platform_device *pdev) > +{ > + struct watchdog_device *wdd = platform_get_drvdata(pdev); > + > + watchdog_unregister_device(wdd); > + > + return 0; > +} > + > +static const struct of_device_id smcwd_dt_ids[] = { > + { .compatible = "arm,smc-wdt" }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, smcwd_dt_ids); > + > +static struct platform_driver smcwd_driver = { > + .probe = smcwd_probe, > + .remove = smcwd_remove, > + .shutdown = smcwd_shutdown, > + .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 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-02-14 14:10 ` Guenter Roeck @ 2020-02-14 21:32 ` Julius Werner 2020-02-15 1:39 ` Evan Benn 2020-02-20 6:50 ` Evan Benn 1 sibling, 1 reply; 24+ messages in thread From: Julius Werner @ 2020-02-14 21:32 UTC (permalink / raw) To: Guenter Roeck Cc: Evan Benn, LKML, Julius Werner, Bjorn Andersson, Mauro Carvalho Chehab, Marcin Juszkiewicz, Catalin Marinas, Olof Johansson, Leonard Crestez, Jonathan Cameron, Dinh Nguyen, linux-arm-kernel, Greg Kroah-Hartman, Will Deacon, linux-watchdog, Rob Herring, Wim Van Sebroeck, Clément Péron, Shawn Guo, David S. Miller, Anson Huang > > with a Secure Monitor firmware to forward watchdog operations to > > 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. > > > > As written, one would assume this to work on all systems implementing > ARM secure firmware, which is not the case. Please select a different > name, and provide information about the systems where this is actually > supported. > > If it happens to be standardized, we will need a reference to the standard > supported. This needs to distinguish from IMX_SC_WDT, which also supports > a secure monitor based watchdog (but doesn't claim to be generic). Back when I wrote this I was hoping it could be something that other platforms can pick up if they want to, but that hasn't happened yet and the code on the Trusted Firmware side is still MediaTek-specific. Unfortunately Arm doesn't make it easy to write generic SMC interfaces and my attempts to change that haven't been very fruitful for now. So yes, probably makes sense to treat this as MediaTek-specific for now, we can still consider expanding it later if there's interest from other platforms. (I would like to avoid every vendor writing their own driver and SMC interface for this, although looking at that IMX driver it seems that we're already there.) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-02-14 21:32 ` Julius Werner @ 2020-02-15 1:39 ` Evan Benn 0 siblings, 0 replies; 24+ messages in thread From: Evan Benn @ 2020-02-15 1:39 UTC (permalink / raw) To: Julius Werner Cc: Guenter Roeck, Evan Benn, LKML, Bjorn Andersson, Mauro Carvalho Chehab, Marcin Juszkiewicz, Catalin Marinas, Olof Johansson, Leonard Crestez, Jonathan Cameron, Dinh Nguyen, linux-arm-kernel, Greg Kroah-Hartman, Will Deacon, linux-watchdog, Rob Herring, Wim Van Sebroeck, Clément Péron, Shawn Guo, David S. Miller, Anson Huang > > As written, one would assume this to work on all systems implementing > > ARM secure firmware, which is not the case. Please select a different > > name, and provide information about the systems where this is actually > > supported. > Back when I wrote this I was hoping it could be something that other > platforms can pick up if they want to, but that hasn't happened yet > and the code on the Trusted Firmware side is still MediaTek-specific. Thanks, I will re-word as mediatek,mt8173-smc-wdt, and address other comments. In the event this does become a standard arm watchdog interface, I assume but do not know that it will be straightforward to change the name here. I am not sure how to proceed with modifying Julius' authored patch in kernel preferred way. I can add myself as co-authored-by and modify patch 2, or add a patch 3 to make preferred changes. I will use approach 2 for now unless otherwise advised. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-02-14 14:10 ` Guenter Roeck 2020-02-14 21:32 ` Julius Werner @ 2020-02-20 6:50 ` Evan Benn 2020-02-20 15:52 ` Guenter Roeck 1 sibling, 1 reply; 24+ messages in thread From: Evan Benn @ 2020-02-20 6:50 UTC (permalink / raw) To: Guenter Roeck Cc: LKML, Julius Werner, Bjorn Andersson, Mauro Carvalho Chehab, Marcin Juszkiewicz, Catalin Marinas, Olof Johansson, Leonard Crestez, Jonathan Cameron, Dinh Nguyen, linux-arm-kernel, Greg Kroah-Hartman, Will Deacon, linux-watchdog, Rob Herring, Wim Van Sebroeck, Clément Péron, Shawn Guo, David S. Miller, Anson Huang > > + if ((int)res->a0 == PSCI_RET_NOT_SUPPORTED) > > + return -ENOTSUPP; > > -ENODEV would be better here. > > > + if ((int)res->a0 == PSCI_RET_INVALID_PARAMS) > > + return -EINVAL; > > + if ((int)res->a0 < 0) > > + return -EIO; In fixing this I found drivers/firmware/psci/psci.c:145 Which also translates psci codes to errno codes, but uses EOPNOTSUPP: switch (errno) { case PSCI_RET_SUCCESS: return 0; case PSCI_RET_NOT_SUPPORTED: return -EOPNOTSUPP; case PSCI_RET_INVALID_PARAMS: case PSCI_RET_INVALID_ADDRESS: return -EINVAL; case PSCI_RET_DENIED: return -EPERM; }; return -EINVAL; Are these more appropriate? Evan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver 2020-02-20 6:50 ` Evan Benn @ 2020-02-20 15:52 ` Guenter Roeck 0 siblings, 0 replies; 24+ messages in thread From: Guenter Roeck @ 2020-02-20 15:52 UTC (permalink / raw) To: Evan Benn Cc: LKML, Julius Werner, Bjorn Andersson, Mauro Carvalho Chehab, Marcin Juszkiewicz, Catalin Marinas, Olof Johansson, Leonard Crestez, Jonathan Cameron, Dinh Nguyen, linux-arm-kernel, Greg Kroah-Hartman, Will Deacon, linux-watchdog, Rob Herring, Wim Van Sebroeck, Clément Péron, Shawn Guo, David S. Miller, Anson Huang On Thu, Feb 20, 2020 at 05:50:03PM +1100, Evan Benn wrote: > > > + if ((int)res->a0 == PSCI_RET_NOT_SUPPORTED) > > > + return -ENOTSUPP; > > > > -ENODEV would be better here. > > > > > + if ((int)res->a0 == PSCI_RET_INVALID_PARAMS) > > > + return -EINVAL; > > > + if ((int)res->a0 < 0) > > > + return -EIO; > > In fixing this I found drivers/firmware/psci/psci.c:145 > Which also translates psci codes to errno codes, but uses EOPNOTSUPP: > > switch (errno) { > case PSCI_RET_SUCCESS: > return 0; > case PSCI_RET_NOT_SUPPORTED: > return -EOPNOTSUPP; > case PSCI_RET_INVALID_PARAMS: > case PSCI_RET_INVALID_ADDRESS: > return -EINVAL; > case PSCI_RET_DENIED: > return -EPERM; > }; > > return -EINVAL; > > Are these more appropriate? > It is customary for driver probe functions to return -ENODEV if the device is not supported. I don't see a reason why this driver should behave any different. "but this other driver does it" is never a good argument. Having said that, yet, with the exception of -EOPNOTSUPP the other return values would be more appropriate. Guenter ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2020-03-13 16:12 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-14 6:26 [PATCH 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Evan Benn 2020-02-14 6:26 ` [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible Evan Benn 2020-02-19 22:30 ` Rob Herring 2020-02-19 23:04 ` Julius Werner 2020-02-19 23:20 ` Guenter Roeck 2020-02-20 6:41 ` Evan Benn 2020-02-20 15:43 ` Guenter Roeck 2020-02-21 15:36 ` Xingyu Chen 2020-02-21 19:41 ` Julius Werner 2020-02-21 20:46 ` Guenter Roeck 2020-02-22 4:01 ` Xingyu Chen 2020-02-24 1:10 ` Evan Benn 2020-02-25 1:23 ` Julius Werner 2020-02-25 7:44 ` Xingyu Chen 2020-03-10 1:00 ` Evan Benn [not found] ` <ad28aa47-0490-937f-898f-0e4695ef6ec6@amlogic.com> 2020-03-11 19:24 ` Julius Werner 2020-03-13 16:13 ` Xingyu Chen 2020-02-14 6:26 ` [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn 2020-02-14 9:35 ` Enric Balletbo Serra 2020-02-14 14:10 ` Guenter Roeck 2020-02-14 21:32 ` Julius Werner 2020-02-15 1:39 ` Evan Benn 2020-02-20 6:50 ` Evan Benn 2020-02-20 15:52 ` Guenter Roeck
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).