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 { > > >>> > > >> > >
