| Message ID | 1550472539-16590-2-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add i.MX8QXP system controller watchdog support | expand |
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.txt > @@ -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. > + }; > }; > }; > > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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.txt >> @@ -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 ? Thanks, Guenter
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.txt > >> @@ -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. Anson. > > Thanks, > Guenter
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.txt >>>> @@ -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. 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. Guenter > Anson. > >> >> Thanks, >> Guenter
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.txt > >>>> @@ -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. > > 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. Thanks, Anson > > Guenter > > > Anson. > > > >> > >> Thanks, > >> Guenter
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter > Roeck> > 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.txt > >> @@ -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 ? > Per my understanding, if the protocol is unlike to change, it can be instantiated by portent node's driver. Hope Rob can help give a more general explanation. Regards Dong Aisheng > Thanks, > Guenter
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.txt > > >>>> @@ -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
On Tue, Feb 26, 2019 at 03:34:12PM -0600, Rob Herring wrote: > > > > > > 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. > It requires extra code where an added DT node would accomplish the same. It requires a mix of DT nodes for existing devices plus extra code for newly added devices. To me that looks like a revert to old platform code, which was replaced with DT descriptions over the last several years. But then if that is where things are going, who am I to argue. Guenter
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
Hi Rob, > > > 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. > The watchdog node here in question actually is not using SCU firmware call. Due to security requirement by SCU, watchdog can only be accessed in security mode, for IMX case, via ARM Trust Firmware. That means the watchdog used in Linux actually is using ARM SMC call and does not depend SCU driver. So It would be strange for SCU driver to instantiate it. For this situation, do you think we can move watchdog out of scu node? Maybe rename the compatible string like "fsl,imx8qxp-sip-watchdog" because it's actually a watchdog serviced by ATF firmware. Regards Dong Aisheng
+Jens W On Thu, Mar 7, 2019 at 6:22 AM Aisheng Dong <aisheng.dong@nxp.com> wrote: > > Hi Rob, > > > > > 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. > > > > The watchdog node here in question actually is not using SCU firmware call. > Due to security requirement by SCU, watchdog can only be accessed in > security mode, for IMX case, via ARM Trust Firmware. That means the > watchdog used in Linux actually is using ARM SMC call and does not > depend SCU driver. So It would be strange for SCU driver to instantiate it. > > For this situation, do you think we can move watchdog out of scu node? > Maybe rename the compatible string like "fsl,imx8qxp-sip-watchdog" > because it's actually a watchdog serviced by ATF firmware. Yes, but that creates more questions. What exactly does ATF talk to for the watchdog? The SCU firmware? Maybe ATF should define and provide a standard watchdog interface? It is still a question of making the firmware discoverable rather than trying to describe the firmware in DT. Rob
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
On Wed, Mar 13, 2019 at 3:11 AM Anson Huang <anson.huang@nxp.com> wrote: > > Hi, Rob > Do you have any feedback about adding imx scu watchdog node in dts? Thanks. Yes, in my reply 2 days ago. Rob
Hi, Rob Best Regards! Anson Huang > -----Original Message----- > From: Rob Herring [mailto:robh@kernel.org] > Sent: 2019年3月12日 5:26 > To: Aisheng Dong <aisheng.dong@nxp.com> > Cc: Anson Huang <anson.huang@nxp.com>; 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; linux- > kernel@vger.kernel.org; kernel@pengutronix.de; olof@lixom.net; > shawnguo@kernel.org; Jens Wiklander <jens.wiklander@linaro.org> > Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog > binding > > +Jens W > > On Thu, Mar 7, 2019 at 6:22 AM Aisheng Dong <aisheng.dong@nxp.com> > wrote: > > > > Hi Rob, > > > > > > > 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. > > > > > > > The watchdog node here in question actually is not using SCU firmware call. > > Due to security requirement by SCU, watchdog can only be accessed in > > security mode, for IMX case, via ARM Trust Firmware. That means the > > watchdog used in Linux actually is using ARM SMC call and does not > > depend SCU driver. So It would be strange for SCU driver to instantiate it. > > > > For this situation, do you think we can move watchdog out of scu node? > > Maybe rename the compatible string like "fsl,imx8qxp-sip-watchdog" > > because it's actually a watchdog serviced by ATF firmware. > > Yes, but that creates more questions. What exactly does ATF talk to for the > watchdog? The SCU firmware? Yes, ATF talks to SCU firmware directly, Linux kernel watchdog driver call SMC instructions to send command to ATF, and ATF will call SCU firmware API to finish the operation requested by Linux kernel watchdog driver. > > Maybe ATF should define and provide a standard watchdog interface? It is > still a question of making the firmware discoverable rather than trying to > describe the firmware in DT. The SMC call by Linux kernel watchdog already follow the SIP(silicon provider) standard, each SoC can define its own protocol for SIP. ATF does NOT have a standard common watchdog interface now, since it is more like a platform specific feature, most of platforms can control watchdog directly from Linux kernel I think. So, do you have suggestion for this case? Either find a place in DT to put watchdog node, or make it a build-in device inside SCU driver? Or you have other better ideas? Thanks, Anson. > > Rob
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 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"; + }; }; };
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(+)