[v5,2/4] dt-bindings: watchdog: add new binding for meson secure watchdog
diff mbox series

Message ID 1576153187-28378-3-git-send-email-xingyu.chen@amlogic.com
State Changes Requested
Headers show
Series
  • add meson secure watchdog driver
Related show

Checks

Context Check Description
robh/dt-meta-schema fail build log
robh/checkpatch success

Commit Message

Xingyu Chen Dec. 12, 2019, 12:19 p.m. UTC
The binding targets the Meson-A/C series compatible SoCs, in which the
watchdog registers are in secure world.

Signed-off-by: Xingyu Chen <xingyu.chen@amlogic.com>
---
 .../bindings/watchdog/amlogic,meson-sec-wdt.yaml   | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/amlogic,meson-sec-wdt.yaml

Comments

Martin Blumenstingl Dec. 12, 2019, 8:05 p.m. UTC | #1
Hi Xingyu and Rob,

On Thu, Dec 12, 2019 at 1:20 PM Xingyu Chen <xingyu.chen@amlogic.com> wrote:
[...]
> +examples:
> +  - |
> +    watchdog {
> +          compatible = "amlogic,meson-sec-wdt";
> +          timeout-sec = <60>;
> +    };
in v3 of this patch Rob commented that there shouldn't be an OF node
if there are no additional properties
with timeout-sec there's now an additional property so my
understanding is that it's fine to have an OF node

what I don't understand yet is where this node should be placed.
is it supposed to be a child node of the secure monitor node (for
which we already have a binding here:
Documentation/devicetree/bindings/firmware/meson/meson_sm.txt) or
where else would we place it inside the .dts?


Martin


[0] https://patchwork.kernel.org/patch/11211399/
Xingyu Chen Dec. 16, 2019, 6:03 a.m. UTC | #2
Hi, Martin

Sorry for the late reply.

On 2019/12/13 4:05, Martin Blumenstingl wrote:
> Hi Xingyu and Rob,
>
> On Thu, Dec 12, 2019 at 1:20 PM Xingyu Chen <xingyu.chen@amlogic.com> wrote:
> [...]
>> +examples:
>> +  - |
>> +    watchdog {
>> +          compatible = "amlogic,meson-sec-wdt";
>> +          timeout-sec = <60>;
>> +    };
> in v3 of this patch Rob commented that there shouldn't be an OF node
> if there are no additional properties
> with timeout-sec there's now an additional property so my
> understanding is that it's fine to have an OF node
Your understanding is correct.
>
> what I don't understand yet is where this node should be placed.
> is it supposed to be a child node of the secure monitor node (for
> which we already have a binding here:
> Documentation/devicetree/bindings/firmware/meson/meson_sm.txt) or
> where else would we place it inside the .dts?
IMO,  Although the watchdog node need to reference the meson_sm node, 
there is no
bus-like dependencies between the devices which the two nodes 
corresponding to.
so i think that the watchdog node as child node of meson_sm maybe not 
appropriate.
>
>
> Martin
>
>
> [0] https://patchwork.kernel.org/patch/11211399/
>
> .
>
Guenter Roeck Dec. 16, 2019, 1:30 p.m. UTC | #3
On 12/15/19 10:03 PM, Xingyu Chen wrote:
> Hi, Martin
> 
> Sorry for the late reply.
> 
> On 2019/12/13 4:05, Martin Blumenstingl wrote:
>> Hi Xingyu and Rob,
>>
>> On Thu, Dec 12, 2019 at 1:20 PM Xingyu Chen <xingyu.chen@amlogic.com> wrote:
>> [...]
>>> +examples:
>>> +  - |
>>> +    watchdog {
>>> +          compatible = "amlogic,meson-sec-wdt";
>>> +          timeout-sec = <60>;
>>> +    };
>> in v3 of this patch Rob commented that there shouldn't be an OF node
>> if there are no additional properties
>> with timeout-sec there's now an additional property so my
>> understanding is that it's fine to have an OF node
> Your understanding is correct.
>>
>> what I don't understand yet is where this node should be placed.
>> is it supposed to be a child node of the secure monitor node (for
>> which we already have a binding here:
>> Documentation/devicetree/bindings/firmware/meson/meson_sm.txt) or
>> where else would we place it inside the .dts?
> IMO,  Although the watchdog node need to reference the meson_sm node, there is no
> bus-like dependencies between the devices which the two nodes corresponding to.
> so i think that the watchdog node as child node of meson_sm maybe not appropriate.

The watchdog driver needs the meson SM's dt node, and it depends on the existence
of that node. That seems enough of a relationship to warrant having it as child note.

Guenter
Xingyu Chen Dec. 18, 2019, 7:37 a.m. UTC | #4
Hi, Guenter Martin

On 2019/12/16 21:30, Guenter Roeck wrote:
> On 12/15/19 10:03 PM, Xingyu Chen wrote:
>> Hi, Martin
>>
>> Sorry for the late reply.
>>
>> On 2019/12/13 4:05, Martin Blumenstingl wrote:
>>> Hi Xingyu and Rob,
>>>
>>> On Thu, Dec 12, 2019 at 1:20 PM Xingyu Chen 
>>> <xingyu.chen@amlogic.com> wrote:
>>> [...]
>>>> +examples:
>>>> +  - |
>>>> +    watchdog {
>>>> +          compatible = "amlogic,meson-sec-wdt";
>>>> +          timeout-sec = <60>;
>>>> +    };
>>> in v3 of this patch Rob commented that there shouldn't be an OF node
>>> if there are no additional properties
>>> with timeout-sec there's now an additional property so my
>>> understanding is that it's fine to have an OF node
>> Your understanding is correct.
>>>
>>> what I don't understand yet is where this node should be placed.
>>> is it supposed to be a child node of the secure monitor node (for
>>> which we already have a binding here:
>>> Documentation/devicetree/bindings/firmware/meson/meson_sm.txt) or
>>> where else would we place it inside the .dts?
>> IMO,  Although the watchdog node need to reference the meson_sm node, 
>> there is no
>> bus-like dependencies between the devices which the two nodes 
>> corresponding to.
>> so i think that the watchdog node as child node of meson_sm maybe not 
>> appropriate.
>
> The watchdog driver needs the meson SM's dt node, and it depends on 
> the existence
> of that node. That seems enough of a relationship to warrant having it 
> as child note.
Thanks for your reply, if i take the wdt node as child of secure monitor 
(sm), how should
i register or find the wdt device ?

I only think of the following three methods :
1). update the sm driver,and scan&register wdt device when the sm driver 
probes(It is like i2c), but there
are too many changes involved.

2). add "simple-bus" key string to compatible of sm node, and it will 
make the child node is registered as
platform device, but it seems that the key string is not match current 
scene.

secure-monitor {
     compatible = "amlogic,meson-gxbb-sm",  "simple-bus";

     watchdog {
         compatible = "amlogic,meson-sec-wdt";
         timeout-sec = <60>;
     }
}

3).  don't register device, and find directly the watchdog node by using 
the of_* API in watchdog
driver (Eg: linux-4.x/drivers/tee/optee/core.c)

secure-monitor {
     compatible = "amlogic,meson-gxbb-sm";

     watchdog {
         compatible = "amlogic,meson-sec-wdt";
         timeout-sec = <60>;
     }
}

The method 3 looks better for me, do you have a better suggestion ? Thanks

BR
>
> Guenter
>
> .
>
Rob Herring Dec. 18, 2019, 8:34 p.m. UTC | #5
On Wed, Dec 18, 2019 at 03:37:49PM +0800, Xingyu Chen wrote:
> Hi, Guenter Martin
> 
> On 2019/12/16 21:30, Guenter Roeck wrote:
> > On 12/15/19 10:03 PM, Xingyu Chen wrote:
> > > Hi, Martin
> > > 
> > > Sorry for the late reply.
> > > 
> > > On 2019/12/13 4:05, Martin Blumenstingl wrote:
> > > > Hi Xingyu and Rob,
> > > > 
> > > > On Thu, Dec 12, 2019 at 1:20 PM Xingyu Chen
> > > > <xingyu.chen@amlogic.com> wrote:
> > > > [...]
> > > > > +examples:
> > > > > +  - |
> > > > > +    watchdog {
> > > > > +          compatible = "amlogic,meson-sec-wdt";
> > > > > +          timeout-sec = <60>;
> > > > > +    };
> > > > in v3 of this patch Rob commented that there shouldn't be an OF node
> > > > if there are no additional properties
> > > > with timeout-sec there's now an additional property so my
> > > > understanding is that it's fine to have an OF node
> > > Your understanding is correct.
> > > > 
> > > > what I don't understand yet is where this node should be placed.
> > > > is it supposed to be a child node of the secure monitor node (for
> > > > which we already have a binding here:
> > > > Documentation/devicetree/bindings/firmware/meson/meson_sm.txt) or
> > > > where else would we place it inside the .dts?
> > > IMO,  Although the watchdog node need to reference the meson_sm
> > > node, there is no
> > > bus-like dependencies between the devices which the two nodes
> > > corresponding to.
> > > so i think that the watchdog node as child node of meson_sm maybe
> > > not appropriate.
> > 
> > The watchdog driver needs the meson SM's dt node, and it depends on the
> > existence
> > of that node. That seems enough of a relationship to warrant having it
> > as child note.
> Thanks for your reply, if i take the wdt node as child of secure monitor
> (sm), how should
> i register or find the wdt device ?
> 
> I only think of the following three methods :
> 1). update the sm driver,and scan&register wdt device when the sm driver
> probes(It is like i2c), but there
> are too many changes involved.

Just add of_platform_default_populate() call and clean-up calls. That's 
not what I'd call 'too many changes'.

 
> 2). add "simple-bus" key string to compatible of sm node, and it will make
> the child node is registered as
> platform device, but it seems that the key string is not match current
> scene.

You previously said it's not a bus...

> 
> secure-monitor {
>     compatible = "amlogic,meson-gxbb-sm",  "simple-bus";
> 
>     watchdog {
>         compatible = "amlogic,meson-sec-wdt";
>         timeout-sec = <60>;
>     }
> }
> 
> 3).  don't register device, and find directly the watchdog node by using the
> of_* API in watchdog
> driver (Eg: linux-4.x/drivers/tee/optee/core.c)
> 
> secure-monitor {
>     compatible = "amlogic,meson-gxbb-sm";
> 
>     watchdog {
>         compatible = "amlogic,meson-sec-wdt";
>         timeout-sec = <60>;
>     }
> }
> 
> The method 3 looks better for me, do you have a better suggestion ? Thanks
> 
> BR
> > 
> > Guenter
> > 
> > .
> >
Xingyu Chen Dec. 19, 2019, 2:51 a.m. UTC | #6
Hi, Rob

On 2019/12/19 4:34, Rob Herring wrote:
> On Wed, Dec 18, 2019 at 03:37:49PM +0800, Xingyu Chen wrote:
>> Hi, Guenter Martin
>>
>> On 2019/12/16 21:30, Guenter Roeck wrote:
>>> On 12/15/19 10:03 PM, Xingyu Chen wrote:
>>>> Hi, Martin
>>>>
>>>> Sorry for the late reply.
>>>>
>>>> On 2019/12/13 4:05, Martin Blumenstingl wrote:
>>>>> Hi Xingyu and Rob,
>>>>>
>>>>> On Thu, Dec 12, 2019 at 1:20 PM Xingyu Chen
>>>>> <xingyu.chen@amlogic.com> wrote:
>>>>> [...]
>>>>>> +examples:
>>>>>> +  - |
>>>>>> +    watchdog {
>>>>>> +          compatible = "amlogic,meson-sec-wdt";
>>>>>> +          timeout-sec = <60>;
>>>>>> +    };
>>>>> in v3 of this patch Rob commented that there shouldn't be an OF node
>>>>> if there are no additional properties
>>>>> with timeout-sec there's now an additional property so my
>>>>> understanding is that it's fine to have an OF node
>>>> Your understanding is correct.
>>>>> what I don't understand yet is where this node should be placed.
>>>>> is it supposed to be a child node of the secure monitor node (for
>>>>> which we already have a binding here:
>>>>> Documentation/devicetree/bindings/firmware/meson/meson_sm.txt) or
>>>>> where else would we place it inside the .dts?
>>>> IMO,  Although the watchdog node need to reference the meson_sm
>>>> node, there is no
>>>> bus-like dependencies between the devices which the two nodes
>>>> corresponding to.
>>>> so i think that the watchdog node as child node of meson_sm maybe
>>>> not appropriate.
>>> The watchdog driver needs the meson SM's dt node, and it depends on the
>>> existence
>>> of that node. That seems enough of a relationship to warrant having it
>>> as child note.
>> Thanks for your reply, if i take the wdt node as child of secure monitor
>> (sm), how should
>> i register or find the wdt device ?
>>
>> I only think of the following three methods :
>> 1). update the sm driver,and scan&register wdt device when the sm driver
>> probes(It is like i2c), but there
>> are too many changes involved.
> Just add of_platform_default_populate() call and clean-up calls. That's
> not what I'd call 'too many changes'.
Thanks for your guidance.
>
>   
>> 2). add "simple-bus" key string to compatible of sm node, and it will make
>> the child node is registered as
>> platform device, but it seems that the key string is not match current
>> scene.
> You previously said it's not a bus...
>
>> secure-monitor {
>>      compatible = "amlogic,meson-gxbb-sm",  "simple-bus";
>>
>>      watchdog {
>>          compatible = "amlogic,meson-sec-wdt";
>>          timeout-sec = <60>;
>>      }
>> }
>>
>> 3).  don't register device, and find directly the watchdog node by using the
>> of_* API in watchdog
>> driver (Eg: linux-4.x/drivers/tee/optee/core.c)
>>
>> secure-monitor {
>>      compatible = "amlogic,meson-gxbb-sm";
>>
>>      watchdog {
>>          compatible = "amlogic,meson-sec-wdt";
>>          timeout-sec = <60>;
>>      }
>> }
>>
>> The method 3 looks better for me, do you have a better suggestion ? Thanks
>>
>> BR
>>> Guenter
>>>
>>> .
>>>
> .
>

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/watchdog/amlogic,meson-sec-wdt.yaml b/Documentation/devicetree/bindings/watchdog/amlogic,meson-sec-wdt.yaml
new file mode 100644
index 00000000..c0ff2ec
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/amlogic,meson-sec-wdt.yaml
@@ -0,0 +1,35 @@ 
+# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+# Copyright (c) 2019 Amlogic, Inc
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/power/amlogic,meson-sec-wdt.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Amlogic Meson Secure Watchdog Timer
+
+maintainers:
+  - Xingyu Chen <xingyu.chen@amlogic.com>
+
+description: |+
+  Secure Watchdog Timer used in Meson-A/C series Compatible SoCs
+
+properties:
+  compatible:
+    enum:
+      - amlogic,meson-sec-wdt
+
+  timeout-sec:
+    minimum: 1
+    maximum: 65
+    description:
+      Watchdog timeout values (in seconds)
+
+required:
+  - compatible
+
+examples:
+  - |
+    watchdog {
+          compatible = "amlogic,meson-sec-wdt";
+          timeout-sec = <60>;
+    };