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@xxxxxxxxxx> > Cc: Guenter Roeck <linux@xxxxxxxxxxxx>; mark.rutland@xxxxxxx; > ulf.hansson@xxxxxxxxxx; heiko@xxxxxxxxx; catalin.marinas@xxxxxxx; > will.deacon@xxxxxxx; bjorn.andersson@xxxxxxxxxx; festevam@xxxxxxxxx; > jagan@xxxxxxxxxxxxxxxxxxxx; Andy Gross <andy.gross@xxxxxxxxxx>; dl- > linux-imx <linux-imx@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux- > watchdog@xxxxxxxxxxxxxxx; arnd@xxxxxxxx; marc.w.gonzalez@xxxxxxx; > s.hauer@xxxxxxxxxxxxxx; enric.balletbo@xxxxxxxxxxxxx; > horms+renesas@xxxxxxxxxxxx; wim@xxxxxxxxxxxxxxxxxx; Daniel Baluta > <daniel.baluta@xxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Aisheng > Dong <aisheng.dong@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; > kernel@xxxxxxxxxxxxxx; olof@xxxxxxxxx; shawnguo@xxxxxxxxxx > 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@xxxxxxxxxx] > > Sent: 2019年2月27日 5:34 > > To: Anson Huang <anson.huang@xxxxxxx> > > Cc: Guenter Roeck <linux@xxxxxxxxxxxx>; mark.rutland@xxxxxxx; > > ulf.hansson@xxxxxxxxxx; heiko@xxxxxxxxx; catalin.marinas@xxxxxxx; > > will.deacon@xxxxxxx; bjorn.andersson@xxxxxxxxxx; festevam@xxxxxxxxx; > > jagan@xxxxxxxxxxxxxxxxxxxx; Andy Gross <andy.gross@xxxxxxxxxx>; dl- > > linux-imx <linux-imx@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux- > > watchdog@xxxxxxxxxxxxxxx; arnd@xxxxxxxx; marc.w.gonzalez@xxxxxxx; > > s.hauer@xxxxxxxxxxxxxx; enric.balletbo@xxxxxxxxxxxxx; > > horms+renesas@xxxxxxxxxxxx; wim@xxxxxxxxxxxxxxxxxx; Daniel Baluta > > <daniel.baluta@xxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Aisheng > > Dong <aisheng.dong@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; > > kernel@xxxxxxxxxxxxxx; olof@xxxxxxxxx; shawnguo@xxxxxxxxxx > > 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@xxxxxxx> > > wrote: > > > > > > Hi, Guenter > > > > > > Best Regards! > > > Anson Huang > > > > > > > -----Original Message----- > > > > From: Guenter Roeck [mailto:groeck7@xxxxxxxxx] On Behalf Of > > > > Guenter Roeck > > > > Sent: 2019年2月24日 11:20 > > > > To: Anson Huang <anson.huang@xxxxxxx>; Rob Herring > > <robh@xxxxxxxxxx> > > > > Cc: mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx; > > > > s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; > festevam@xxxxxxxxx; > > > > catalin.marinas@xxxxxxx; will.deacon@xxxxxxx; > > > > wim@xxxxxxxxxxxxxxxxxx; Aisheng Dong <aisheng.dong@xxxxxxx>; > > > > ulf.hansson@xxxxxxxxxx; Daniel Baluta <daniel.baluta@xxxxxxx>; > > > > Andy Gross <andy.gross@xxxxxxxxxx>; > > > > horms+renesas@xxxxxxxxxxxx; heiko@xxxxxxxxx; arnd@xxxxxxxx; > > > > bjorn.andersson@xxxxxxxxxx; jagan@xxxxxxxxxxxxxxxxxxxx; > > > > enric.balletbo@xxxxxxxxxxxxx; marc.w.gonzalez@xxxxxxx; > > > > olof@xxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > > > > linux-kernel@xxxxxxxxxxxxxxx; linux-arm- > > > > kernel@xxxxxxxxxxxxxxxxxxx; linux-watchdog@xxxxxxxxxxxxxxx; > > > > dl-linux-imx <linux-imx@xxxxxxx> > > > > 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@xxxxxxxxx] On Behalf Of > > > > >> Guenter Roeck > > > > >> Sent: 2019年2月24日 1:08 > > > > >> To: Rob Herring <robh@xxxxxxxxxx>; Anson Huang > > > > <anson.huang@xxxxxxx> > > > > >> Cc: mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx; > > > > >> s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; > > > > >> festevam@xxxxxxxxx; catalin.marinas@xxxxxxx; > > will.deacon@xxxxxxx; > > > > >> wim@linux- > > > > watchdog.org; > > > > >> Aisheng Dong <aisheng.dong@xxxxxxx>; ulf.hansson@xxxxxxxxxx; > > > > >> Daniel Baluta <daniel.baluta@xxxxxxx>; Andy Gross > > > > >> <andy.gross@xxxxxxxxxx>; > > > > >> horms+renesas@xxxxxxxxxxxx; heiko@xxxxxxxxx; arnd@xxxxxxxx; > > > > >> bjorn.andersson@xxxxxxxxxx; jagan@xxxxxxxxxxxxxxxxxxxx; > > > > >> enric.balletbo@xxxxxxxxxxxxx; marc.w.gonzalez@xxxxxxx; > > > > >> olof@xxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > > > > >> linux-kernel@xxxxxxxxxxxxxxx; linux-arm- > > > > >> kernel@xxxxxxxxxxxxxxxxxxx; linux-watchdog@xxxxxxxxxxxxxxx; > > > > >> dl-linux-imx <linux-imx@xxxxxxx> > > > > >> 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@xxxxxxx> > > > > >>>> --- > > > > >>>> 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
