Hi, Guenter

Best Regards!
Anson Huang

-----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 <robh@kernel.org>
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; 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; 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.txt

@@ -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 {
   			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 "
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.

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.

Thanks,
Anson

Guenter

quoted

Anson.

quoted

Thanks,
Guenter