RE: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding

Hi, Rob
	Do you have any feedback about adding imx scu watchdog node in dts? Thanks.

Best Regards!
Anson Huang

-----Original Message-----
From: Anson Huang
Sent: 2019年3月6日 22:45
To: 'Rob Herring' <robh@kernel.org>
Cc: Guenter Roeck <linux@roeck-us.net>; mark.rutland@arm.com;
ulf.hansson@linaro.org; heiko@sntech.de; catalin.marinas@arm.com;
will.deacon@arm.com; bjorn.andersson@linaro.org; festevam@gmail.com;
jagan@amarulasolutions.com; Andy Gross [off-list ref]; dl-
linux-imx [off-list ref]; devicetree@vger.kernel.org; linux-
watchdog@vger.kernel.org; arnd@arndb.de; marc.w.gonzalez@free.fr;
s.hauer@pengutronix.de; enric.balletbo@collabora.com;
horms+renesas@verge.net.au; wim@linux-watchdog.org; Daniel Baluta
[off-list ref]; linux-arm-kernel@lists.infradead.org; Aisheng
Dong [off-list ref]; linux-kernel@vger.kernel.org;
kernel@pengutronix.de; olof@lixom.net; shawnguo@kernel.org
Subject: RE: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog
binding

Hi, Rob
	Sorry to bring back this topic again about whether to put "imx-sc-
wdt" node inside DT's SCU node, after some discussion with my team, there
is case of virtualization, disabling "imx-sc-wdt" for Linux OS, while enable it
for Android OS running together on same SoC, then Linux needs to disable
watchdog from its DTB while Android can enable it. For such kind of scenario,
do you think it is reasonable to have "imx-sc-wdt" node inside SCU node in
DT? We do NOT have good idea for this scenario if imx-sc-wdt is added as
built-in platform device inside SCU driver.

Best Regards!
Anson Huang

quoted

-----Original Message-----
From: Rob Herring [mailto:robh@kernel.org]
Sent: 2019年2月27日 5:34
To: Anson Huang <redacted>
Cc: Guenter Roeck <linux@roeck-us.net>; mark.rutland@arm.com;
ulf.hansson@linaro.org; heiko@sntech.de; catalin.marinas@arm.com;
will.deacon@arm.com; bjorn.andersson@linaro.org; festevam@gmail.com;
jagan@amarulasolutions.com; Andy Gross [off-list ref]; dl-
linux-imx [off-list ref]; devicetree@vger.kernel.org; linux-
watchdog@vger.kernel.org; arnd@arndb.de; marc.w.gonzalez@free.fr;
s.hauer@pengutronix.de; enric.balletbo@collabora.com;
horms+renesas@verge.net.au; wim@linux-watchdog.org; Daniel Baluta
[off-list ref]; linux-arm-kernel@lists.infradead.org; Aisheng
Dong [off-list ref]; linux-kernel@vger.kernel.org;
kernel@pengutronix.de; olof@lixom.net; shawnguo@kernel.org
Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog
binding

On Sun, Feb 24, 2019 at 8:26 PM Anson Huang [off-list ref]
wrote:

quoted

Hi, Guenter

Best Regards!
Anson Huang

quoted

-----Original Message-----
From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of
Guenter Roeck
Sent: 2019年2月24日 11:20
To: Anson Huang <redacted>; Rob Herring

[off-list ref]

quoted

quoted

Cc: mark.rutland@arm.com; shawnguo@kernel.org;
s.hauer@pengutronix.de; kernel@pengutronix.de;

festevam@gmail.com;

quoted

quoted

quoted

catalin.marinas@arm.com; will.deacon@arm.com;
wim@linux-watchdog.org; Aisheng Dong [off-list ref];
ulf.hansson@linaro.org; Daniel Baluta [off-list ref];
Andy Gross [off-list ref];
horms+renesas@verge.net.au; heiko@sntech.de; arnd@arndb.de;
bjorn.andersson@linaro.org; jagan@amarulasolutions.com;
enric.balletbo@collabora.com; marc.w.gonzalez@free.fr;
olof@lixom.net; devicetree@vger.kernel.org;
linux-kernel@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; linux-watchdog@vger.kernel.org;
dl-linux-imx [off-list ref]
Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add
watchdog binding

On 2/23/19 7:04 PM, Anson Huang wrote:

quoted

Hi, Guenter/Rob

Best Regards!
Anson Huang

quoted

-----Original Message-----
From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of
Guenter Roeck
Sent: 2019年2月24日 1:08
To: Rob Herring <robh@kernel.org>; Anson Huang

[off-list ref]

quoted

quoted

Cc: mark.rutland@arm.com; shawnguo@kernel.org;
s.hauer@pengutronix.de; kernel@pengutronix.de;
festevam@gmail.com; catalin.marinas@arm.com;

will.deacon@arm.com;

quoted

quoted

quoted

quoted

wim@linux-

watchdog.org;

quoted

quoted

Aisheng Dong [off-list ref]; ulf.hansson@linaro.org;
Daniel Baluta [off-list ref]; Andy Gross
[off-list ref];
horms+renesas@verge.net.au; heiko@sntech.de; arnd@arndb.de;
bjorn.andersson@linaro.org; jagan@amarulasolutions.com;
enric.balletbo@collabora.com; marc.w.gonzalez@free.fr;
olof@lixom.net; devicetree@vger.kernel.org;
linux-kernel@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; linux-watchdog@vger.kernel.org;
dl-linux-imx [off-list ref]
Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add
watchdog binding

On 2/22/19 11:52 AM, Rob Herring wrote:

quoted

On Mon, Feb 18, 2019 at 06:53:48AM +0000, Anson Huang wrote:

quoted

Add i.MX8QXP system controller watchdog binding.

Signed-off-by: Anson Huang <redacted>
---
Changes since V1:
 - update dts node name to "watchdog";
---

Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
| 10

++++++++++

quoted

quoted

   1 file changed, 10 insertions(+)

diff --git
a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
index 4b79751..f388ec6 100644
---
a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt

+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu
+++ .t
+++ xt

@@ -136,6 +136,12 @@ Required properties:
                            resource id for thermal driver to

get temperature

via

quoted

quoted

                            SCU IPC.

+Watchdog bindings based on SCU Message Protocol
+------------------------------------------------------------
+
+Required properties:
+- compatible: should be "fsl,imx8qxp-sc-wdt";
+
   Example (imx8qxp):
   -------------
   lsio_mu1: mailbox@5d1c0000 { @@ -188,6 +194,10 @@

firmware

quoted

{

quoted

quoted

quoted

quoted

quoted

quoted

                         tsens-num = <1>;
                         #thermal-sensor-cells = <1>;
                 };
+
+                watchdog: watchdog {
+                        compatible = "fsl,imx8qxp-sc-wdt";

As-is, there's no reason for this to be in DT. The parent
node's driver can instantiate the wdog.

As the driver is currently written, you are correct, since it
doesn't accept watchdog timeout configuration through devicetree.

Question is if that is intended. Is it ?

I am a little confused, do you mean we no need to have "watchdog"
node

in side "scu"

quoted

node? Or we need to modify the watchdog node's compatible string

to "

quoted

quoted

quoted

quoted

fsl,imx-sc-wdt" to make it more generic for other platforms? If
yes, I can

resend the patch series to modify it.

quoted

I think Rob suggested that the SCU parent driver should
instantiate the watchdog without explicit watchdog node. That
would be possible, but it currently uses
devm_of_platform_populate() to do the instantiation, and changing
that would be a mess. Besides, it does sem to me that your
suggested node would describe the hardware, so I am not sure I
understand the

reasoning.

It would just be a call to create a platform device instead. How is that a

mess?

quoted

It's describing firmware. We have DT for describing h/w we've failed
to make discoverable. We should not repeat that and just describe

firmware in DT.

quoted

Make the firmware discoverable! Though there are cases like firmware
provided clocks where we still need something in DT, but this is not
one of them.

quoted

quoted

For my part I referred to
      watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT, &pdev-

quoted

dev); in the driver, which guarantees that the timeout property
will not be

used to set the timeout. A more common implementation would have
been

      imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
      ret = watchdog_init_timeout(imx_sc_wdd, timeout,
&pdev->dev);

where 'timeout' is the module parameter. Which is actually not
used in your driver.
Hmm ... I wasn't careful enough with my review. The timeout
initialization as- is doesn't make sense. I'll comment on that in the patch.

I understand now, in our cases, I would still prefer to have
watchdog node under the SCU parent node, since there could be other
property setting difference between different i.MX platforms with
system controller watchdog inside, using the SCU node to instantiate
makes us a little confused about the watchdog, so if it is NOT that
critical, I think we should keep watchdog node. But to make the
watchdog driver more generic for other i.MX platforms, I changed the
compatible string to

"fsl,imx-sc-wdt" in driver, and each SoC should has it as fallback if
it can reuse this watchdog driver.

You handle differences between SoCs by having specific compatibles. So
"fsl,imx-sc-wdt" moves in the wrong direction assuming we have a node
in the first place.

Rob