Re: [PATCH v2] arm64: dts: mt8173-oak: Switch to SMC watchdog

[Pin-yen Lin]

On Fri, Jul 29, 2022 at 10:59 AM Pin-yen Lin <treapking@xxxxxxxxxxxx> wrote:
>
> On Thu, Jul 28, 2022 at 11:51 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> >
> > Il 28/07/22 17:39, Pin-yen Lin ha scritto:
> > > On Thu, Jul 28, 2022 at 7:21 PM AngeloGioacchino Del Regno
> > > <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> > >>
> > >> Il 27/07/22 11:40, Pin-yen Lin ha scritto:
> > >>> Switch to SMC watchdog because we need direct control of HW watchdog
> > >>> registers from kernel. The corresponding firmware was uploaded in
> > >>> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405.
> > >>>
> > >>> Signed-off-by: Pin-yen Lin <treapking@xxxxxxxxxxxx>
> > >>> ---
> > >>>
> > >>> Changes in v2:
> > >>> - Move the modifications to mt8173-elm.dtsi and add some comments.
> > >>>
> > >>>    arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 12 ++++++++++++
> > >>>    1 file changed, 12 insertions(+)
> > >>>
> > >>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> > >>> index e21feb85d822..b2269770abc3 100644
> > >>> --- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> > >>> +++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> > >>> @@ -161,6 +161,18 @@ hdmi_connector_in: endpoint {
> > >>>                        };
> > >>>                };
> > >>>        };
> > >>> +
> > >>> +     soc {
> > >>> +             /*
> > >>> +              * Disable the original MMIO watch dog and switch to the SMC watchdog,
> > >>> +              * which operates on the same MMIO.
> > >>> +              */
> > >>> +             /delete-node/ watchdog@10007000;
> > >>
> > >> Unfortunately, we're not quite there yet.
> > >> The comment is fine, but...
> > >>
> > >> There's no need to /delete-node/: you can just do it like
> > >>
> > >> /*
> > >>    * Disable the original MMIO watch dog and switch to the SMC watchdog,
> > >>    * which operates on the same MMIO.
> > >>    */
> > >> &watchdog {
> > >>          status = "disabled";
> > >> };
> > >>
> > >> and...
> > >>
> > >>> +
> > >>> +             watchdog {
> > >>
> > >> This isn't addressable, hence it belongs to the root node, not to soc.
> > >> If you did that because of naming issues, I would propose to call it
> > >> smc-watchdog instead of watchdog.
> > >>
> > >>
> > >>> +                     compatible = "arm,smc-wdt";
> > >>
> > > Thanks for the suggestion. I'll modify it accordingly in v3.
> > >
> > >> P.S.: No timeout-sec?
> > >
> > > The example in the binding file has a timeout-sec property, but it is
> > > not defined in the binding nor used in the driver...
> > > The driver seems to talk with the firmware to get a timeout value[1]
> > > instead of reading it from the devicetree.
> > >
> >
> > Oh. I admit I trusted the binding blindly, didn't check the actual driver code.
> >
> > On a note, we should check why is that binding partially wrong and eventually
> > fix it (remove the property), or add the capability to the driver, but feel free
> > to ignore that for now, as this is not relevant for the context of this specific
> > change that you're trying to do here.
>
> It looks like the timeout-sec property was not defined nor referenced
> from the very first commit [2][3]. I'll send out another patch to
> remove it.
>
> [2]: dt-bindings:
> https://lore.kernel.org/all/20200505131242.v6.1.Id96574f1f52479d7a2f3b866b8a0552ab8c03d7f@changeid/
> [3]: watchdog driver:
> https://lore.kernel.org/all/20200505131242.v6.2.Ia92bb4d4ce84bcefeba1d00aaa1c1e919b6164ef@changeid/

I checked the drivers again, and I realized that the `timeout-sec`
property overrides the default timeout from the firmware.

But for MT8173 chromebooks, I think we can just leave it blank and use
the default value.

> >
> > P.S.: I just noticed that the commit title is also wrong. s/mt8173-oak/mt8173-elm/g
>
> I sent out the v3 before I saw this reply.  I'll send out a v4 for
> this and address Krzysztof's comment as well.
>
> >
> > Waiting for a v3!
> >
> >
> > > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/arm_smc_wdt.c#L138
> > >>
> > >> Regards,
> > >> Angelo
> > >>
> > >>> +             };
> > >>> +     };
> > >>>    };
> > >>>
> > >>>    &mfg_async {
> > >>>
> > >>
> >