Re: [PATCH 8/9] riscv: dts: gf: Initial support for the P8700 Boston board

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `riscv,isa-extensions` array omits the required 'zicbom' extension and is not sorted alphanumerically.
- [High] Mismatch between PCI 'bus-range' and memory-mapped ECAM 'reg' size allows out-of-bounds memory access.
- [Low] Missing `#include <dt-bindings/gpio/gpio.h>` and usage of magic number instead of macro for GPIO flags.
- [Low] Missing mandatory `device_type = "pci"` and `ranges` properties in PCI-to-PCI bridge nodes.
--

commit 79e1dd46e7add2552901143ed6ac7196cb0914df
Author: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>

riscv: dts: gf: Initial support for the P8700 Boston board

This patch adds the initial device tree for the MIPS P8700 and the Boston
FPGA development board. It includes the corresponding bitfile configurations,
Makefile, and Kconfig updates.

> diff --git a/arch/riscv/boot/dts/gf/p8700-boston.dts b/arch/riscv/boot/dts/gf/p8700-boston.dts
> new file mode 100644
> index 0000000000000..b829899592aee
> --- /dev/null
> +++ b/arch/riscv/boot/dts/gf/p8700-boston.dts

[ ... ]

> +	pci2: pci@14000000 {
> +		compatible = "xlnx,axi-pcie-host-1.00.a";
> +		device_type = "pci";
> +		reg = <0x14000000 0x2000000>;
> +
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		#interrupt-cells = <1>;
> +
> +		interrupt-parent = <&aplic_s0>;
> +		interrupts = <7 IRQ_TYPE_LEVEL_HIGH>;
> +
> +		ranges = <0x02000000 0 0x16000000
> +			  0x16000000 0 0x100000>;
> +
> +		bus-range = <0x00 0xff>;

[Severity: High]
Can this mismatch between bus-range and memory-mapped ECAM size allow an
out-of-bounds memory access?

The defined reg maps only 32MB of ECAM configuration space, which safely
covers buses 0 to 31. However, bus-range advertises up to 256 buses to the
PCI core. If the PCI core attempts to enumerate buses 32 or higher (e.g.,
during hotplug or if a bridge's subordinate bus is configured > 31), it will
generate an offset past the 32MB mapping.

Should bus-range be restricted to <0x00 0x1f>, or should the reg size be
expanded to 256MB?

[ ... ]

> +		pci2_root@0,0 {
> +			compatible = "pci10ee,7021", "pci-bridge";
> +			reg = <0x00000000 0 0 0 0>;
> +
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +
> +			eg20t_bridge@1,0,0 {
> +				compatible = "pci8086,8800", "pci-bridge";
> +				reg = <0x00010000 0 0 0 0>;
> +
> +				#address-cells = <3>;
> +				#size-cells = <2>;

[Severity: Low]
Do these nested PCI bridge nodes (pci2_root@0,0 and eg20t_bridge@1,0,0) need
the mandatory device_type = "pci" and ranges properties?

Standard bindings for PCI bridges require these properties for correct OF
tree parsing, even if Linux typically enumerates them via config space reads.

> +				eg20t_mac@2,0,1 {
> +					compatible = "pci8086,8802",
> +						     "intel,pch-gbe";
> +					reg = <0x00020100 0 0 0 0>;
> +					phy-reset-gpios = <&eg20t_gpio 6 1>;

[Severity: Low]
Can this magic number 1 be replaced with the GPIO_ACTIVE_LOW macro?

Including <dt-bindings/gpio/gpio.h> and using the macro would improve
readability and adhere to standard GPIO flag conventions.

> diff --git a/arch/riscv/boot/dts/gf/p8700.dtsi b/arch/riscv/boot/dts/gf/p8700.dtsi
> new file mode 100644
> index 0000000000000..ba1854df5201b
> --- /dev/null
> +++ b/arch/riscv/boot/dts/gf/p8700.dtsi

[ ... ]

> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "gf,mips-p8700", "riscv";
> +			reg = <0x0>;
> +			mmu-type = "riscv,sv39";
> +			riscv,cbom-block-size = <64>;
> +			riscv,isa-base = "rv64i";
> +			riscv,isa-extensions = "i", "m", "a", "f", "d", "c",
> +					       "zicntr", "zicsr", "zifencei",
> +					       "zihpm", "zba", "zbb",
> +					       "xmipsexectl";

[Severity: High]
Will the omission of the "zicbom" string in riscv,isa-extensions break DMA
cache flushing for non-coherent devices?

The CPU nodes define riscv,cbom-block-size = <64>, indicating hardware
support for Cache Block Management. Without "zicbom" present in the
extensions array, the RISC-V kernel feature framework will not set
RISCV_ISA_EXT_ZICBOM. This causes the ALT_CMO_OP macro to compile as NOPs,
which could result in corrupted transfers for devices requiring non-coherent
DMA, like pci2.

Also, should the multi-letter extensions be sorted alphanumerically to comply
with Devicetree bindings (e.g., placing "zba" and "zbb" before "zicntr")?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-p8700-dts-v1-0-3267f14ebce1@htecgroup.com?part=8