Re: [PATCH] powerpc/dts: Add dts for WatchGuard Firebox T30

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] powerpc/dts: missing machine descriptor for watchguard,firebox-t30 hangs boot
- [Medium] powerpc/dts: /delete-property/ used on CPU child node in firebox-t30.dts
- [Medium] powerpc/dts: out-of-tree DT properties and undocumented binding in firebox-t30
- [Low] powerpc/dts: invalid /include/ paths in firebox-t30.dts break dtc build
--

commit 14186547f28b2b27ac2d22fcbaad845f34789ba3
Author: Lefteris Koutrakis <lefteris.koutrakis@gmail.com>

powerpc/dts: Add dts for WatchGuard Firebox T30

This commit adds a device tree source file and Makefile entry for the
WatchGuard Firebox T30 and T30-W security appliances based on the Freescale
P1020 SoC.

> diff --git a/arch/powerpc/boot/dts/fsl/Makefile b/arch/powerpc/boot/dts/fsl/Makefile
> index d2cc8e1f007ea..ebbf301e1a8fc 100644
> --- a/arch/powerpc/boot/dts/fsl/Makefile
> +++ b/arch/powerpc/boot/dts/fsl/Makefile
> @@ -3,6 +3,7 @@
>  dtb-$(CONFIG_OF_ALL_DTBS) := $(patsubst $(src)/%.dts,%.dtb, $(wildcard $(src)/*.dts))
>  
>  # MPC85xx platforms
> +dtb-$(CONFIG_MPC85xx_RDB) += firebox-t30.dtb
>  dtb-$(CONFIG_BSC9131_RDB) += bsc9131rdb.dtb
>  dtb-$(CONFIG_BSC9132_QDS) += bsc9132qds.dtb
>  dtb-$(CONFIG_C293_PCIE) += c293pcie.dtb
> diff --git a/arch/powerpc/boot/dts/fsl/firebox-t30.dts b/arch/powerpc/boot/dts/fsl/firebox-t30.dts
> new file mode 100644
> index 0000000000000..546e989384b76
> --- /dev/null
> +++ b/arch/powerpc/boot/dts/fsl/firebox-t30.dts
> @@ -0,0 +1,413 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
> +
> +/include/ "fsl/p1020si-pre.dtsi"

[Severity: Low]
Will dtc be able to resolve "fsl/p1020si-pre.dtsi" (and
"fsl/p1020si-post.dtsi" at the bottom of the file) when building from
arch/powerpc/boot/dts/fsl/?

In scripts/Makefile.dtbs, cmd_dtc invokes dtc with -i $(dir $<) (which is
arch/powerpc/boot/dts/fsl/) and -i scripts/dtc/include-prefixes:

    $(DTC) -o $@ -b 0 $(addprefix -i,$(dir $<) $(DTC_INCLUDE)) \
           $(DTC_FLAGS) -d $(depfile).dtc.tmp $(dtc-tmp) ; \

When srcfile_relative_open() -> fopen_any_on_path() in scripts/dtc/srcpos.c
resolves /include/ "fsl/p1020si-pre.dtsi" relative to
arch/powerpc/boot/dts/fsl/, it looks for
arch/powerpc/boot/dts/fsl/fsl/p1020si-pre.dtsi and fails the build when
CONFIG_MPC85xx_RDB or CONFIG_OF_ALL_DTBS is enabled.

Should both /include/ directives drop the "fsl/" prefix to match the other
device trees in arch/powerpc/boot/dts/fsl/?

> +
> +/ {
> +	model = "Watchguard Firebox T30";
> +	compatible = "watchguard,firebox-t30";

[Severity: High]
How does the kernel match a PowerPC machine descriptor when booting with
this device tree?

The root node sets compatible = "watchguard,firebox-t30", and
arch/powerpc/boot/dts/fsl/Makefile builds firebox-t30.dtb under
CONFIG_MPC85xx_RDB. However, arch/powerpc/platforms/85xx/mpc85xx_rdb.c only
defines machine descriptors for specific Freescale RDB compatible strings
such as "fsl,P1020RDB":

arch/powerpc/platforms/85xx/mpc85xx_rdb.c:
    define_machine(p1020_rdb) {
        .name           = "P1020 RDB",
        .compatible     = "fsl,P1020RDB",
        ...
    };

Without a matching define_machine() entry for "watchguard,firebox-t30" in
arch/powerpc/platforms/85xx/, setup_arch() -> probe_machine() in
arch/powerpc/kernel/setup-common.c reaches &__machine_desc_end and halts
boot in an infinite loop:

    if (machine_id >= &__machine_desc_end) {
        pr_err("No suitable machine description found !\n");
        for (;;);
    }

> +
> +	chosen {
> +		bootargs-override = "console=ttyS0,115200 rw root=/dev/mmcblk0p2 rootfstype=squashfs rootwait";
> +	};
> +
> +	aliases {
> +		label-mac-device = &enet0;
> +		led-boot = &led0;
> +		led-failsafe = &led1;
> +		led-running = &led0;
> +		led-upgrade = &led1;
> +	};
> +
> +	memory {
> +		device_type = "memory";
> +	};
> +
> +	cpus {
> +		/delete-property/ PowerPC,P1020@1;
> +	};

[Severity: Medium]
Should this use /delete-node/ instead of /delete-property/ to remove
PowerPC,P1020@1?

In arch/powerpc/boot/dts/fsl/p1020si-pre.dtsi, PowerPC,P1020@1 is defined as
a child node under /cpus rather than a property:

    cpus {
        ...
        PowerPC,P1020@1 {
            device_type = "cpu";
            reg = <0x1>;
            next-level-cache = <&L2>;
        };
    };

In scripts/dtc/livetree.c, merge_nodes() handles /delete-property/ by
calling delete_property_by_name(), which only walks node->proplist and
leaves child nodes in node->children untouched. As a result, the second CPU
node remains present in the compiled DTB.

[ ... ]

> +		sdhc@2e000 {
> +			compatible = "fsl,p1020-esdhc", "fsl,esdhc";
> +			sdhci,auto-cmd12;
> +			reg = <0x2e000 0x1000>;
> +			interrupts = <0x48 0x2 0x0 0x0>;
> +			interrupt-parent = <&mpic>;
> +			fsl,sdhci-dma-broken;
> +			fsl,sdhci-ahb2mag-irq-bypass;
> +			fsl,sdhci-adjust-timeout;
> +			clock-frequency = <0>;
> +		};

[Severity: Medium]
Are these downstream OpenWrt properties in /chosen, /aliases, and
sdhc@2e000 intended for mainline?

In /chosen, early_init_dt_scan_chosen() in drivers/of/fdt.c only reads
"bootargs" and ignores "bootargs-override":

    p = of_get_flat_dt_prop(node, "bootargs", &l);
    if (p != NULL && l > 0)
        strscpy(cmdline, p, min(l, COMMAND_LINE_SIZE));

In sdhc@2e000, the properties fsl,sdhci-dma-broken,
fsl,sdhci-ahb2mag-irq-bypass, and fsl,sdhci-adjust-timeout are not parsed
by drivers/mmc/host/sdhci-of-esdhc.c and are not permitted by
Documentation/devicetree/bindings/mmc/fsl,esdhc.yaml (which sets
unevaluatedProperties: false), while the standard properties in this node
duplicate what p1020si-post.dtsi already includes via pq3-esdhc-0.dtsi.

Similarly, the /aliases entries (label-mac-device, led-boot, led-failsafe,
led-running, led-upgrade) and the root compatible string
"watchguard,firebox-t30" are not documented under
Documentation/devicetree/bindings/.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028102158.71032-1-lefteris.koutrakis@gmail.com?part=1