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

From: Anson Huang
Date: Wed Mar 13 2019 - 04:12:55 EST

Next message: Rasmus Villemoes: "Re: [PATCH] Makefile: Add '-fno-builtin-bcmp' to CLANG_FLAGS"
Previous message: Dmitry Vyukov: "Re: KASAN: null-ptr-deref Read in reclaim_high"
In reply to: Anson Huang: "RE: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding"
Next in thread: Rob Herring: "Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Rasmus Villemoes: "Re: [PATCH] Makefile: Add '-fno-builtin-bcmp' to CLANG_FLAGS"
Previous message: Dmitry Vyukov: "Re: KASAN: null-ptr-deref Read in reclaim_high"
In reply to: Anson Huang: "RE: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding"
Next in thread: Rob Herring: "Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]