lkml.org 
[lkml]   [2022]   [Jul]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2] arm64: dts: mt8173-oak: Switch to SMC watchdog
On Thu, Jul 28, 2022 at 11:51 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> 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@collabora.com> 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@chromium.org>
> >>> ---
> >>>
> >>> 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/
>
> 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 {
> >>>
> >>
>

\
 
 \ /
  Last update: 2022-07-29 05:01    [W:0.076 / U:1.636 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site