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

From: AngeloGioacchino Del Regno
Date: Mon Jul 25 2022 - 06:31:54 EST


Il 25/07/22 12:19, Pin-yen Lin ha scritto:
On Mon, Jul 25, 2022 at 4:39 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:

Il 25/07/22 10:24, 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.


There's a fundamental issue with this change, I think.

What happens if we run this devicetree on a device that does *not* have
the new(er) firmware?

I haven't tried this patch with an older firmware. I'll manage to
build one for this.

The kernel *shall not* get broken when running on devices that are running
on older firmware, especially because that's what was initially supported
and what is working right now.

Actually the current approach does not work *right*. The device boots,
but the watchdog does not work properly.


Is this a Chromebook firmware specific issue?

Also, all MT8173 ChromeOS devices have this firmware updated, and we
don't have other upstream users apart from mt8173-evb. Do we want to
support the developers that are running upstream linux with their
MT8173 boards?


Upstream shall not be just about one machine: if we add support for a SoC there,
we shall support the SoC-generic things in the SoC-specific DTSI, and the machine
specific things in the machine-specific devicetrees.

Chromebooks are not the only machines using the MT8173 SoC (Chuwi, Amazon also do
have products using MT8173), so we shall not make the main mt8173.dtsi incompatible
with these machines.


For this reason, I think that we should get some code around that checks
if the SMC watchdog is supported and, if not, resort to MMIO wdog.

What is the expected way to support this backward compatibility? Do we
put the old compatible strings ("mediatek,mt8173-wdt" and
"mediatek,mt6589-wdt") after "arm,smc-wdt" and reject it in the
drivers if the firmware does not support it?

I don't know what's the best option to support both cases... Perhaps a good one
would be to check (in mtk_wdt? or in arm_smc_wdt?) if the arm_smc_wdt is actually
supported in firmware, so if the SMC one is registered, we skip the other.


Regards,
Angelo


Signed-off-by: Pin-yen Lin <treapking@xxxxxxxxxxxx>
---

arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index a2aef5aa67c1..2d1c776740a5 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -528,10 +528,8 @@ power-domain@MT8173_POWER_DOMAIN_MFG {
};
};

- watchdog: watchdog@10007000 {
- compatible = "mediatek,mt8173-wdt",
- "mediatek,mt6589-wdt";
- reg = <0 0x10007000 0 0x100>;
+ watchdog {
+ compatible = "arm,smc-wdt";
};

timer: timer@10008000 {