Re: [PATCH v1 1/1] ARM: dts: draeger: Add devicetree files for M48 Intel/Altera SoCFPGA Cyclone

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The `gpio3` and `gpio4` controllers are improperly placed under the `soc` node instead of `base_fpga_region`.
- [Low] Invalid format for top-level `compatible` string (`draeger,M48/socfpga`).
- [Low] Node unit addresses contain a leading '0x'.
- [Low] Invalid and deprecated properties used in DW APB GPIO port nodes (`bank-name` and `snps,nr-gpios`).
- [Low] Undocumented `cdns,page-size` and `cdns,block-size` properties in the QSPI flash node.
- [Low] Invalid SPI NOR flash node name and compatible string.
- [Low] Comment in the aliases node contains typos and references the wrong GMAC node.
--

--- Patch [1]: [PATCH v1 1/1] ARM: dts: draeger: Add devicetree files for M48 Intel/Altera SoCFPGA Cyclone ---
commit e37d3a1ced17e7ebd85060af457e4c1448b9946c
Author: Petko Manolov <petko.manolov@konsulko.com>

ARM: dts: draeger: Add devicetree files for M48 Intel/Altera SoCFPGA Cyclone

This commit introduces device tree files for the Draeger M48 Cyclone V SoC.

> diff --git a/arch/arm/boot/dts/intel/socfpga/socfpga_cyclone5_draeger_m48.dts b/arch/arm/boot/dts/intel/socfpga/socfpga_cyclone5_draeger_m48.dts
> new file mode 100644
> index 0000000000000..fd597f15c0d8d
> --- /dev/null
> +++ b/arch/arm/boot/dts/intel/socfpga/socfpga_cyclone5_draeger_m48.dts
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  (C) Copyright 2020 Draeger and Licensors, info@draeger.com
> + */
> +#include "socfpga_cyclone5.dtsi"
> +
> +/ {
> +	model = "Draeger M48 Cyclone V SoC";
> +	compatible = "draeger,M48/socfpga", "altr,socfpga-cyclone5", "altr,socfpga";

[Severity: Low]
Does this compatible string format cause dtbs_check validation warnings?

The forward slash is typically invalid in compatible strings according to the
DeviceTree specification, and the "draeger" vendor prefix appears to be
undocumented.

[ ... ]

> +	aliases {
> +	/* this allow the ethaddr uboot environmnet variable contents
> +	 * to be added to the gmac1 device tree blob.
> +	 */
> +		ethernet0 = &gmac0;

[Severity: Low]
Is this comment referencing the correct node?

It mentions "gmac1" but the alias maps to "gmac0". Additionally, is
"environmnet" a typo?

> +	};
> +
> +	soc {
> +		gpio3: gpio@0xff202000 {

[Severity: High]
Could placing this node under the soc bus lead to an AXI bus stall and system
crash?

The address for gpio3 maps to the Lightweight HPS-to-FPGA (LWHPS2FPGA) bridge.
By placing it under the generic soc node, the platform bus will probe it at
boot. If the FPGA is unprogrammed and the bridge hasn't been released from
reset, or if it is dynamically reprogrammed later, won't the driver's initial
I/O read crash the kernel?

Should these nodes be placed under a base_fpga_region node instead?

[Severity: Low]
Should the unit address in the node name omit the leading "0x" to avoid DTC
compiler warnings?

> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			compatible = "snps,dw-apb-gpio";
> +			reg = <0xff202000 0x20>;
> +			status = "okay";
> +
> +			portd: gpio-controller@0 {
> +				compatible = "snps,dw-apb-gpio-port";
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				snps,nr-gpios = <2>;
> +				reg = <0>;
> +				bank-name = "SYNC_I";

[Severity: Low]
Are these properties compliant with the current schema?

The "bank-name" property appears to be undocumented, and "snps,nr-gpios" has
been deprecated in favor of "ngpios".

> +			};
> +		};
> +
> +		gpio4: gpio@0xff202020 {

[Severity: High]
As with gpio3, could placing this LWHPS2FPGA bridge device under the generic
soc bus cause an AXI bus stall if the FPGA is unprogrammed?

[Severity: Low]
Does this node name also need the "0x" prefix removed from its unit address?

> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			compatible = "snps,dw-apb-gpio";
> +			reg = <0xff202020 0x20>;
> +			status = "okay";
> +
> +			porte: gpio-controller@0 {
> +				compatible = "snps,dw-apb-gpio-port";
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				snps,nr-gpios = <2>;
> +				reg = <0>;
> +				bank-name = "SYNC_O";

[Severity: Low]
Should this node also use "ngpios" instead of "snps,nr-gpios", and drop the
undocumented "bank-name" property?

[ ... ]

> +&qspi {
> +	status = "okay";
> +
> +	flash0: n25q00@0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "n25q00";

[Severity: Low]
Does this compatible string need a vendor prefix like "micron,n25q00" and a
standard fallback like "jedec,spi-nor" to pass schema validation?

Additionally, should the node name use a generic name like "flash@0" instead
of "n25q00@0"?

> +		reg = <0>;
> +
> +		/* chip select */
> +		spi-max-frequency = <100000000>;
> +		m25p,fast-read;
> +		cdns,page-size = <256>;
> +		cdns,block-size = <16>;

[Severity: Low]
Are these cdns properties documented in the upstream bindings?

It looks like they might be ignored by the spi-cadence-quadspi driver and
cause schema validation errors.

> +		cdns,read-delay = <4>;
> +		cdns,tshsl-ns = <50>;
> +		cdns,tsd2d-ns = <50>;
> +		cdns,tchsh-ns = <4>;
> +		cdns,tslch-ns = <4>;
> +	};
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825135426.3967333-2-petko.manolov@konsulko.com?part=1