Re: [PATCH v2 5/6] arm64: dts: mediatek: Add basic support for MT8163 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64: dts: mediatek: undocumented and unmatched mt8163-wdt compatible
- [Medium] arm64: dts: mediatek: mt8163: wrong IRQ trigger type on watchdog node
--

commit 67e669b8d381b252cd1f347ad5f9a9a92a7b5322
Author: Ben Grisdale <bengris32@protonmail.ch>

arm64: dts: mediatek: Add basic support for MT8163 SoC

This commit adds the base device tree include file for the MediaTek MT8163
SoC, defining the CPUs, caches, PSCI, interrupt controllers, timers, PMU,
watchdog, and UART nodes.

> diff --git a/arch/arm64/boot/dts/mediatek/mt8163.dtsi b/arch/arm64/boot/dts/mediatek/mt8163.dtsi
> new file mode 100644
> index 0000000000000..645f4e15587bf
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt8163.dtsi
> @@ -0,0 +1,243 @@

[ ... ]

> +	soc {
> +		compatible = "simple-bus";
> +		ranges;
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +
> +		watchdog: watchdog@10007000 {
> +			compatible = "mediatek,mt8163-wdt";

[Severity: Medium]
Does the watchdog@10007000 node fail to bind and fail dtbs_check with only
"mediatek,mt8163-wdt" in its compatible property?

Unlike the timer, sysirq, and uart nodes, "mediatek,mt8163-wdt" is not
documented in Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
and is not present in mtk_wdt_dt_ids[] in drivers/watchdog/mtk_wdt.c:

static const struct of_device_id mtk_wdt_dt_ids[] = {
	{ .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
	{ .compatible = "mediatek,mt6589-wdt", .data = &mt6589_data },
	...
	{ .compatible = "mediatek,mt8195-wdt", .data = &mt8195_data },
	{ /* sentinel */ }
};

Without a fallback compatible such as "mediatek,mt6589-wdt" (and a
corresponding binding entry) or a driver match entry, mtk_wdt_probe() will
not match this node, leaving the hardware watchdog, system restart handler,
and TOPRGU reset controller unavailable. Should this include a fallback
compatible and binding update?

> +			reg = <0 0x10007000 0 0x1000>;
> +			interrupts = <GIC_SPI 128 IRQ_TYPE_EDGE_FALLING>;

[Severity: Medium]
Should the interrupt trigger type on watchdog@10007000 be IRQ_TYPE_LEVEL_LOW
instead of IRQ_TYPE_EDGE_FALLING?

Because the parent interrupt controller is sysirq, mtk_sysirq_set_type() in
drivers/irqchip/irq-mtk-sysirq.c inverts the polarity and forwards
IRQ_TYPE_EDGE_FALLING to the GIC as IRQ_TYPE_EDGE_RISING:

	if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
		if (type == IRQ_TYPE_LEVEL_LOW)
			type = IRQ_TYPE_LEVEL_HIGH;
		else
			type = IRQ_TYPE_EDGE_RISING;
		value |= (1 << offset);
	}

However, the MediaTek TOPRGU watchdog bark interrupt is level-triggered (as
described in commit 311f6c4bba24 ("watchdog: mediatek: acknowledge pretimeout
interrupt") and Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml),
and mtk_wdt_deassert_irq() in drivers/watchdog/mtk_wdt.c acknowledges the
pretimeout interrupt by toggling WDT_MODE_IRQ_LEVEL_EN twice across a delay:

	reg = ioread32(wdt_base + WDT_MODE);
	reg ^= WDT_MODE_IRQ_LEVEL_EN;
	iowrite32(reg | WDT_MODE_KEY, wdt_base + WDT_MODE);

	udelay(WDT_IRQ_LEVEL_SYNC_US);

	reg = ioread32(wdt_base + WDT_MODE);
	reg ^= WDT_MODE_IRQ_LEVEL_EN;
	iowrite32(reg | WDT_MODE_KEY, wdt_base + WDT_MODE);

With edge triggering configured, wouldn't toggling WDT_MODE_IRQ_LEVEL_EN in
mtk_wdt_deassert_irq() cause spurious re-triggering or missed pretimeout
interrupts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-arm64-initial-support-for-mediatek-mt8163-soc-v2-0-e68d7d428f99@protonmail.ch?part=5