RE: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding
Hi, Rob Best Regards! Anson Huang > -Original Message- > From: Rob Herring [mailto:r...@kernel.org] > Sent: 2019年3月12日 5:26 > To: Aisheng Dong > Cc: Anson Huang ; Guenter 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 ; dl- > linux-imx ; 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 > ; linux-arm-ker...@lists.infradead.org; linux- > ker...@vger.kernel.org; ker...@pengutronix.de; o...@lixom.net; > shawn...@kernel.org; Jens Wiklander > 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 > 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
Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding
On Wed, Mar 13, 2019 at 3:11 AM Anson Huang 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
RE: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding
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' > Cc: Guenter Roeck ; 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 ; dl- > linux-imx ; 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 > ; linux-arm-ker...@lists.infradead.org; Aisheng > Dong ; 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 > > Cc: Guenter Roeck ; 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 ; dl- > > linux-imx ; 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 > > ; linux-arm-ker...@lists.infradead.org; Aisheng > > Dong ; 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 > > 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 ; Rob Herring > > > > > > 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 ; > > > > ulf.hans...@linaro.org; Daniel Baluta ; > > > > Andy Gross ; > > > > 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 > > > > 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 ; Anson Huang > > > > > > > > >> 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; > > > >
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 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
RE: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding
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
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 > Cc: Guenter Roeck ; 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 ; dl- > linux-imx ; 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 > ; linux-arm-ker...@lists.infradead.org; Aisheng > Dong ; 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 > 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 ; Rob Herring > > > > 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 ; > > > ulf.hans...@linaro.org; Daniel Baluta ; Andy > > > Gross ; > > > 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 > > > 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 ; Anson Huang > > > > > > >> 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 ; ulf.hans...@linaro.org; > > > >> Daniel Baluta ; Andy Gross > > > >> ; > > > >> 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 > > > >> 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 +, Anson Huang wrote: > > > >>>> Add i.MX8QXP system controller watchdog binding. > > > >>>> > > > >>>> Signed-off-by: Anson Huang > > > >>>> --- > > > >>>> Changes since V1: > > > >>>> - update dts node name to "watchdog"; > > > >>>> --- > > > >>>>Documentation/devicetree/bindings/arm/freescale/
Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding
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
Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding
On Sun, Feb 24, 2019 at 8:26 PM Anson Huang 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 ; Rob Herring
> > 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 ; ulf.hans...@linaro.org; Daniel
> > Baluta ; Andy Gross ;
> > 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
> >
> > 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 ; Anson Huang
> >
> > >> 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 ; ulf.hans...@linaro.org; Daniel
> > >> Baluta ; Andy Gross ;
> > >> 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
> > >> 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 +, Anson Huang wrote:
> > >>>> Add i.MX8QXP system controller watchdog binding.
> > >>>>
> > >>>> Signed-off-by: Anson Huang
> > >>>> ---
> > >>>> 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@5d1c {
> > >>>> @@ -188,6 +194,10 @@ firmware {
> > >>>> tsens-num = <1>;
> > >>>> #thermal-sensor-cells = <1>;
> > >>>> };
> > >>>> +
> > >>>> +watchdog: watchdog {
> > >>>> +compatible = "fsl,imx8qxp-sc-wdt";
> > >>>
> > >>> As-is, there's no rea
RE: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding
> From: Guenter Roeck [mailto:groe...@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 +, Anson Huang wrote:
> >> Add i.MX8QXP system controller watchdog binding.
> >>
> >> Signed-off-by: Anson Huang
> >> ---
> >> 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@5d1c {
> >> @@ -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
RE: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding
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 ; Rob Herring
> 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 ; ulf.hans...@linaro.org; Daniel
> Baluta ; Andy Gross ;
> 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
>
> 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 ; Anson Huang
>
> >> 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 ; ulf.hans...@linaro.org; Daniel
> >> Baluta ; Andy Gross ;
> >> 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
> >> 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 +, Anson Huang wrote:
> >>>> Add i.MX8QXP system controller watchdog binding.
> >>>>
> >>>> Signed-off-by: Anson Huang
> >>>> ---
> >>>> 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@5d1c {
> >>>> @@ -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 w
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 ; Anson Huang
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 ; ulf.hans...@linaro.org; Daniel
Baluta ; Andy Gross ;
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
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 +, Anson Huang wrote:
Add i.MX8QXP system controller watchdog binding.
Signed-off-by: Anson Huang
---
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@5d1c {
@@ -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, >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, >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
RE: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding
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 ; Anson Huang
> 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 ; ulf.hans...@linaro.org; Daniel
> Baluta ; Andy Gross ;
> 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
>
> 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 +, Anson Huang wrote:
> >> Add i.MX8QXP system controller watchdog binding.
> >>
> >> Signed-off-by: Anson Huang
> >> ---
> >> 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@5d1c {
> >> @@ -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
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 +, Anson Huang wrote:
Add i.MX8QXP system controller watchdog binding.
Signed-off-by: Anson Huang
---
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@5d1c {
@@ -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
Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding
On Mon, Feb 18, 2019 at 06:53:48AM +, Anson Huang wrote:
> Add i.MX8QXP system controller watchdog binding.
>
> Signed-off-by: Anson Huang
> ---
> 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@5d1c {
> @@ -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-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
