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

[Anson Huang <anson.huang@nxp.com>]

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 <andy.gross@linaro.org>; dl-
> linux-imx <linux-imx@nxp.com>; 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
> <daniel.baluta@nxp.com>; linux-arm-kernel@lists.infradead.org; Aisheng
> Dong <aisheng.dong@nxp.com>; 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
> 
> > -----Original Message-----
> > From: Rob Herring [mailto:robh@kernel.org]
> > Sent: 2019年2月27日 5:34
> > To: Anson Huang <anson.huang@nxp.com>
> > 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 <andy.gross@linaro.org>; dl-
> > linux-imx <linux-imx@nxp.com>; 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
> > <daniel.baluta@nxp.com>; linux-arm-kernel@lists.infradead.org; Aisheng
> > Dong <aisheng.dong@nxp.com>; 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 <anson.huang@nxp.com>
> > wrote:
> > >
> > > 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 <anson.huang@nxp.com>; 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 <aisheng.dong@nxp.com>;
> > > > ulf.hansson@linaro.org; Daniel Baluta <daniel.baluta@nxp.com>;
> > > > Andy Gross <andy.gross@linaro.org>;
> > > > 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 <linux-imx@nxp.com>
> > > > Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add
> > > > watchdog binding
> > > >
> > > > On 2/23/19 7:04 PM, Anson Huang wrote:
> > > > > Hi, Guenter/Rob
> > > > >
> > > > > Best Regards!
> > > > > Anson Huang
> > > > >
> > > > >> -----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
> > > > <anson.huang@nxp.com>
> > > > >> 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 <aisheng.dong@nxp.com>; ulf.hansson@linaro.org;
> > > > >> Daniel Baluta <daniel.baluta@nxp.com>; Andy Gross
> > > > >> <andy.gross@linaro.org>;
> > > > >> 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 <linux-imx@nxp.com>
> > > > >> Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add
> > > > >> watchdog binding
> > > > >>
> > > > >> On 2/22/19 11:52 AM, Rob Herring wrote:
> > > > >>> On Mon, Feb 18, 2019 at 06:53:48AM +0000, Anson Huang wrote:
> > > > >>>> Add i.MX8QXP system controller watchdog binding.
> > > > >>>>
> > > > >>>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > > >>>> ---
> > > > >>>> Changes since V1:
> > > > >>>>  - update dts node name to "watchdog";
> > > > >>>> ---
> > > > >>>>
> > > > >>>> Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > >>>> | 10
> > > > >> ++++++++++
> > > > >>>>    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
> > > > >>>>                             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"
> > > > > 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.
> > > > >
> > > >
> > > > 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?
> >
> > 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.
> > 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.
> >
> > > >
> > > > For my part I referred to
> > > >       watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT, &pdev-
> > > > >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