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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] powerpc/dts: invalid compatible string in firebox-t30 tpm@29 node
- [Low] powerpc/dts: duplicate firebox-t30.dtb entry in fsl/Makefile
- [Low] powerpc/dts: malformed spi@7000 interrupts and redundant SoC properties
- [Low] powerpc/dts: firebox-t30: undocumented compatible and unsupported DT properties
--

commit 68d1b53d33f85dd0ec26113a393584818efd23a1
Author: Lefteris Koutrakis <lefteris.koutrakis@gmail.com>

powerpc/dts: Add Device Tree for WatchGuard Firebox T30

This commit adds a new Device Tree source file for the WatchGuard Firebox T30
board based on the Freescale P1020 SoC and hooks it into the Freescale PowerPC
DTS Makefile.

> diff --git a/arch/powerpc/boot/dts/fsl/Makefile b/arch/powerpc/boot/dts/fsl/Makefile
> index d2cc8e1f007ea..0a5c8b5967335 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
> @@ -44,3 +45,4 @@ dtb-$(CONFIG_GEF_SBC310) += gef_sbc310.dtb
>  dtb-$(CONFIG_GEF_SBC610) += gef_sbc610.dtb
>  dtb-$(CONFIG_GEF_PPC9A) += gef_ppc9a.dtb
>  dtb-$(CONFIG_MVME7100) += mvme7100.dtb
> +dtb-$(CONFIG_MPC85xx_RDB) += firebox-t30.dtb

[Severity: Low]
Is this second addition of firebox-t30.dtb to dtb-$(CONFIG_MPC85xx_RDB) at
the bottom of arch/powerpc/boot/dts/fsl/Makefile (under the MPC86xx platforms
section) unintended?

Since firebox-t30.dtb is already appended to dtb-$(CONFIG_MPC85xx_RDB) at the
start of the MPC85xx platforms section, building with CONFIG_MPC85xx_RDB=y
causes firebox-t30.dtb to be listed twice in $(dtb-y) and $(obj)/dtbs-list via
scripts/Makefile.dtbs.

> 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..6e9ec7f90ece2
> --- /dev/null
> +++ b/arch/powerpc/boot/dts/fsl/firebox-t30.dts
> @@ -0,0 +1,398 @@
> +// 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/ "p1020si-pre.dtsi"
> +
> +/ {
> +	model = "Watchguard Firebox T30";
> +	compatible = "watchguard,firebox-t30", "fsl,P1020RDB";

[ ... ]

> +	soc: soc@ffe00000 {
> +		ranges = <0x0 0x0 0xffe00000 0x100000>;
> +
> +		i2c@3000 {
> +			rtc@30 {
> +				compatible = "sii,s35390a";
> +				reg = <0x30>;
> +			};
> +		};
> +
> +		i2c@3100 {
> +			tpm@29 {
> +				compatible = "tpm,tpm_i2c_atmel";
> +				reg = <0x29>;
> +			};
> +		};

[Severity: Medium]
Should the compatible property for tpm@29 under i2c@3100 be
"atmel,at97sc3204t" instead of "tpm,tpm_i2c_atmel"?

In drivers/char/tpm/tpm_i2c_atmel.c, the OF match table only matches
"atmel,at97sc3204t":

    static const struct of_device_id i2c_atmel_of_match[] = {
        {.compatible = "atmel,at97sc3204t"},
        {},
    };
    MODULE_DEVICE_TABLE(of, i2c_atmel_of_match);

When an I2C device is instantiated from Device Tree, i2c_device_uevent() in
drivers/i2c/i2c-core-base.c emits only the OF modalias and returns before
emitting the legacy i2c: modalias:

    rc = of_device_uevent_modalias(dev, env);
    if (rc != -ENODEV)
        return rc;

With compatible = "tpm,tpm_i2c_atmel", the kernel emits
MODALIAS=of:NtpmT(null)Ctpm,tpm_i2c_atmel, which prevents automatic module
loading when CONFIG_TCG_TIS_I2C_ATMEL=m and fails DT schema validation against
Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml.

> +
> +		sdhc@2e000 {
> +			compatible = "fsl,p1020-esdhc", "fsl,esdhc";
> +			sdhci,auto-cmd12;
> +			reg = <0x2e000 0x1000>;
> +			interrupts = <0x48 0x2 0x0 0x0>;
> +			interrupt-parent = <&mpic>;
> +			clock-frequency = <0>;
> +		};
> +
> +		spi0: spi@7000 {
> +			compatible = "fsl,mpc8536-espi";
> +			reg = <0x7000 0x1000>;
> +			interrupts = <0x3b 0x2>;
> +			interrupt-parent = <&mpic>;
> +			fsl,espi-num-chipselects = <4>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;

[Severity: Low]
Can the SoC-level properties in sdhc@2e000 and spi0: spi@7000 be dropped here
so they are inherited from p1020si-post.dtsi?

In spi0: spi@7000, interrupts = <0x3b 0x2> only provides 2 cells for &mpic,
whereas mpic: pic@40000 in pq3-mpic.dtsi requires #interrupt-cells = <4>.
Because p1020si-post.dtsi (which includes pq3-espi-0.dtsi and pq3-esdhc-0.dtsi)
is included at the bottom of firebox-t30.dts, dtc overwrites this 2-cell
specifier with the 4-cell interrupts = <59 0x2 0 0> definition from
pq3-espi-0.dtsi.

Furthermore, sdhc@2e000 only repeats properties already defined in
p1020si-post.dtsi and pq3-esdhc-0.dtsi without adding any board-specific
configuration, and spi@7000 only needs to define its flash@0 and slic@1 child
nodes.

[ ... ]

> +		enet0: ethernet@b0000 {
> +			phy-connection-type = "rgmii-id";
> +			rx-internal-delay-ps = <2000>;
> +			tx-internal-delay-ps = <2000>;
> +			nvmem-cells = <&macaddr_nor_cfg0_1830 0>;
> +			nvmem-cell-names = "mac-address";
> +
> +			fixed-link {
> +				speed = <1000>;
> +				full-duplex;
> +			};
> +		};

[ ... ]

> +			ath10k: wifi@0,0 {
> +				reg = <0x0000 0 0 0 0>;
> +				#gpio-cells = <2>;
> +				gpio-controller;
> +				nvmem-cells = <&macaddr_nor_cfg0_1894 0>;
> +				nvmem-cell-names = "mac-address";
> +			};

[Severity: Low]
Are the following bindings and properties in firebox-t30.dts intentional?

1. The root node uses compatible = "watchguard,firebox-t30", "fsl,P1020RDB",
   where neither the "watchguard" vendor prefix (in
   Documentation/devicetree/bindings/vendor-prefixes.yaml) nor the
   "watchguard,firebox-t30" board compatible string is documented.

2. enet0: ethernet@b0000 (an fsl,etsec2 device from pq3-etsec2-0.dtsi) sets
   rx-internal-delay-ps = <2000> and tx-internal-delay-ps = <2000>, which the
   gianfar driver in drivers/net/ethernet/freescale/gianfar.c does not parse
   or support.

3. ath10k: wifi@0,0 declares gpio-controller and #gpio-cells = <2>, which are
   not supported by the ath10k driver or its binding in
   Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml, and are
   not referenced elsewhere in this DTS.

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