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