[PATCH 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[PATCH 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Evan Benn]

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

[PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Evan Benn]

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

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Rob Herring]

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
> 

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Julius Werner]

> 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.

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Guenter Roeck]

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.

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Evan Benn]

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.

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Guenter Roeck]

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.

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Xingyu Chen]

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.
> .

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Julius Werner]

> 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).

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Guenter Roeck]

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

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[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
>
> .

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Evan Benn]

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
> >
> > .

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Julius Werner]

> 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.)

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Xingyu Chen]

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
> .

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Evan Benn]

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
> > .

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Julius Werner]

> - *_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.)

Re: [PATCH 1/2] dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

[Xingyu Chen]

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.
>
> .