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' <r...@kernel.org>
> Cc: Guenter Roeck <li...@roeck-us.net>; mark.rutl...@arm.com;
> ulf.hans...@linaro.org; he...@sntech.de; catalin.mari...@arm.com;
> will.dea...@arm.com; bjorn.anders...@linaro.org; feste...@gmail.com;
> ja...@amarulasolutions.com; Andy Gross <andy.gr...@linaro.org>; dl-
> linux-imx <linux-...@nxp.com>; devicet...@vger.kernel.org; linux-
> watch...@vger.kernel.org; a...@arndb.de; marc.w.gonza...@free.fr;
> s.ha...@pengutronix.de; enric.balle...@collabora.com;
> horms+rene...@verge.net.au; w...@linux-watchdog.org; Daniel Baluta
> <daniel.bal...@nxp.com>; linux-arm-ker...@lists.infradead.org; Aisheng
> Dong <aisheng.d...@nxp.com>; linux-kernel@vger.kernel.org;
> ker...@pengutronix.de; o...@lixom.net; shawn...@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:r...@kernel.org]
> > Sent: 2019年2月27日 5:34
> > To: Anson Huang <anson.hu...@nxp.com>
> > Cc: Guenter Roeck <li...@roeck-us.net>; mark.rutl...@arm.com;
> > ulf.hans...@linaro.org; he...@sntech.de; catalin.mari...@arm.com;
> > will.dea...@arm.com; bjorn.anders...@linaro.org; feste...@gmail.com;
> > ja...@amarulasolutions.com; Andy Gross <andy.gr...@linaro.org>; dl-
> > linux-imx <linux-...@nxp.com>; devicet...@vger.kernel.org; linux-
> > watch...@vger.kernel.org; a...@arndb.de; marc.w.gonza...@free.fr;
> > s.ha...@pengutronix.de; enric.balle...@collabora.com;
> > horms+rene...@verge.net.au; w...@linux-watchdog.org; Daniel Baluta
> > <daniel.bal...@nxp.com>; linux-arm-ker...@lists.infradead.org; Aisheng
> > Dong <aisheng.d...@nxp.com>; linux-kernel@vger.kernel.org;
> > ker...@pengutronix.de; o...@lixom.net; shawn...@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.hu...@nxp.com>
> > wrote:
> > >
> > > Hi, Guenter
> > >
> > > Best Regards!
> > > Anson Huang
> > >
> > > > -----Original Message-----
> > > > From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of
> > > > Guenter Roeck
> > > > Sent: 2019年2月24日 11:20
> > > > To: Anson Huang <anson.hu...@nxp.com>; Rob Herring
> > <r...@kernel.org>
> > > > Cc: mark.rutl...@arm.com; shawn...@kernel.org;
> > > > s.ha...@pengutronix.de; ker...@pengutronix.de;
> feste...@gmail.com;
> > > > catalin.mari...@arm.com; will.dea...@arm.com;
> > > > w...@linux-watchdog.org; Aisheng Dong <aisheng.d...@nxp.com>;
> > > > ulf.hans...@linaro.org; Daniel Baluta <daniel.bal...@nxp.com>;
> > > > Andy Gross <andy.gr...@linaro.org>;
> > > > horms+rene...@verge.net.au; he...@sntech.de; a...@arndb.de;
> > > > bjorn.anders...@linaro.org; ja...@amarulasolutions.com;
> > > > enric.balle...@collabora.com; marc.w.gonza...@free.fr;
> > > > o...@lixom.net; devicet...@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; linux-arm-
> > > > ker...@lists.infradead.org; linux-watch...@vger.kernel.org;
> > > > dl-linux-imx <linux-...@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:groe...@gmail.com] On Behalf Of
> > > > >> Guenter Roeck
> > > > >> Sent: 2019年2月24日 1:08
> > > > >> To: Rob Herring <r...@kernel.org>; Anson Huang
> > > > <anson.hu...@nxp.com>
> > > > >> Cc: mark.rutl...@arm.com; shawn...@kernel.org;
> > > > >> s.ha...@pengutronix.de; ker...@pengutronix.de;
> > > > >> feste...@gmail.com; catalin.mari...@arm.com;
> > will.dea...@arm.com;
> > > > >> wim@linux-
> > > > watchdog.org;
> > > > >> Aisheng Dong <aisheng.d...@nxp.com>; ulf.hans...@linaro.org;
> > > > >> Daniel Baluta <daniel.bal...@nxp.com>; Andy Gross
> > > > >> <andy.gr...@linaro.org>;
> > > > >> horms+rene...@verge.net.au; he...@sntech.de; a...@arndb.de;
> > > > >> bjorn.anders...@linaro.org; ja...@amarulasolutions.com;
> > > > >> enric.balle...@collabora.com; marc.w.gonza...@free.fr;
> > > > >> o...@lixom.net; devicet...@vger.kernel.org;
> > > > >> linux-kernel@vger.kernel.org; linux-arm-
> > > > >> ker...@lists.infradead.org; linux-watch...@vger.kernel.org;
> > > > >> dl-linux-imx <linux-...@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.hu...@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
