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

Anson Huang anson.huang at nxp.com
Wed Mar 6 06:45:03 PST 2019


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 at kernel.org]
> Sent: 2019年2月27日 5:34
> To: Anson Huang <anson.huang at nxp.com>
> Cc: Guenter Roeck <linux at roeck-us.net>; mark.rutland at arm.com;
> ulf.hansson at linaro.org; heiko at sntech.de; catalin.marinas at arm.com;
> will.deacon at arm.com; bjorn.andersson at linaro.org; festevam at gmail.com;
> jagan at amarulasolutions.com; Andy Gross <andy.gross at linaro.org>; dl-
> linux-imx <linux-imx at nxp.com>; devicetree at vger.kernel.org; linux-
> watchdog at vger.kernel.org; arnd at arndb.de; marc.w.gonzalez at free.fr;
> s.hauer at pengutronix.de; enric.balletbo at collabora.com;
> horms+renesas at verge.net.au; wim at linux-watchdog.org; Daniel Baluta
> <daniel.baluta at nxp.com>; linux-arm-kernel at lists.infradead.org; Aisheng
> Dong <aisheng.dong at nxp.com>; linux-kernel at vger.kernel.org;
> kernel at pengutronix.de; olof at lixom.net; shawnguo at 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 at nxp.com>
> wrote:
> >
> > Hi, Guenter
> >
> > Best Regards!
> > Anson Huang
> >
> > > -----Original Message-----
> > > From: Guenter Roeck [mailto:groeck7 at gmail.com] On Behalf Of Guenter
> > > Roeck
> > > Sent: 2019年2月24日 11:20
> > > To: Anson Huang <anson.huang at nxp.com>; Rob Herring
> <robh at kernel.org>
> > > Cc: mark.rutland at arm.com; shawnguo at kernel.org;
> > > s.hauer at pengutronix.de; kernel at pengutronix.de; festevam at gmail.com;
> > > catalin.marinas at arm.com; will.deacon at arm.com;
> > > wim at linux-watchdog.org; Aisheng Dong <aisheng.dong at nxp.com>;
> > > ulf.hansson at linaro.org; Daniel Baluta <daniel.baluta at nxp.com>; Andy
> > > Gross <andy.gross at linaro.org>;
> > > horms+renesas at verge.net.au; heiko at sntech.de; arnd at arndb.de;
> > > bjorn.andersson at linaro.org; jagan at amarulasolutions.com;
> > > enric.balletbo at collabora.com; marc.w.gonzalez at free.fr;
> > > olof at lixom.net; devicetree at vger.kernel.org;
> > > linux-kernel at vger.kernel.org; linux-arm- kernel at lists.infradead.org;
> > > linux-watchdog at vger.kernel.org; dl-linux-imx <linux-imx at 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 at gmail.com] On Behalf Of
> > > >> Guenter Roeck
> > > >> Sent: 2019年2月24日 1:08
> > > >> To: Rob Herring <robh at kernel.org>; Anson Huang
> > > <anson.huang at nxp.com>
> > > >> Cc: mark.rutland at arm.com; shawnguo at kernel.org;
> > > >> s.hauer at pengutronix.de; kernel at pengutronix.de;
> > > >> festevam at gmail.com; catalin.marinas at arm.com;
> will.deacon at arm.com;
> > > >> wim at linux-
> > > watchdog.org;
> > > >> Aisheng Dong <aisheng.dong at nxp.com>; ulf.hansson at linaro.org;
> > > >> Daniel Baluta <daniel.baluta at nxp.com>; Andy Gross
> > > >> <andy.gross at linaro.org>;
> > > >> horms+renesas at verge.net.au; heiko at sntech.de; arnd at arndb.de;
> > > >> bjorn.andersson at linaro.org; jagan at amarulasolutions.com;
> > > >> enric.balletbo at collabora.com; marc.w.gonzalez at free.fr;
> > > >> olof at lixom.net; devicetree at vger.kernel.org;
> > > >> linux-kernel at vger.kernel.org; linux-arm-
> > > >> kernel at lists.infradead.org; linux-watchdog at vger.kernel.org;
> > > >> dl-linux-imx <linux-imx at 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 at 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 at 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


More information about the linux-arm-kernel mailing list