Re: [PATCH 5/9] riscv: dts: ultrarisc: Add initial device tree for UltraRISC DP1000

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The `gpio-ranges` property values in `gpio_b`, `gpio_c`, and `gpio_d` incorrectly specify a local GPIO base that exceeds the available pins.
- [High] PCI I/O space is incorrectly mapped 1:1 to a high CPU address, breaking legacy PCI endpoints.
- [High] The Ethernet node incorrectly defines a broadcast MAC address as its local unicast MAC address.
- [Medium] Mismatch between CPU node unit addresses and their `reg` properties.
- [Medium] Ethernet compatible strings are listed in the wrong order.
- [Low] Clock node names contain underscores instead of hyphens.
--

commit f5aec289ca4daa2ae7f4c27bac4b1694ece4043b
Author: Jia Wang <wangjia@ultrarisc.com>

riscv: dts: ultrarisc: Add initial device tree for UltraRISC DP1000

Add the base device tree for the UltraRISC DP1000 SoC. It describes the
8×CP100 CPU cores and essential SoC peripherals including the interrupt
controller, pinctrl, GPIO, UART, SPI, I2C, PCIe, GMAC and the DMA
controller.

Link: https://lore.kernel.org/lkml/20260427-ultrarisc-pcie-v4-2-98935f6cdfb5@ultrarisc.com/
Link: https://lore.kernel.org/lkml/20260429-ultrarisc-serial-v7-3-e475cce9e274@ultrarisc.com/

> diff --git a/arch/riscv/boot/dts/ultrarisc/dp1000.dtsi b/arch/riscv/boot/dts/ultrarisc/dp1000.dtsi
> new file mode 100644
> index 0000000000000..1aae53fc1a2b1
> --- /dev/null
> +++ b/arch/riscv/boot/dts/ultrarisc/dp1000.dtsi
> @@ -0,0 +1,851 @@
[ ... ]
> +		cpu4: cpu@4 {
> +			compatible = "ultrarisc,cp100", "riscv";
> +			reg = <0x10>;

Does the unit address in the node name here correctly match the first
address specified in its reg property?

The Devicetree Specification mandates that the hexadecimal value after the @
must exactly match the reg value to avoid parser errors and dtc compiler
warnings. Should these be renamed to cpu@10, cpu@11, cpu@12, and cpu@13?

[ ... ]
> +	clocks {
> +		device_clk: device_clk {

Is it intended to use underscores in these clock node names?

Device tree naming conventions generally require node names to use hyphens
instead of underscores (e.g., device-clk) to avoid triggering strict
character warnings.

[ ... ]
> +			gpio_b: gpio-port@1 {
> +				compatible = "snps,dw-apb-gpio-port";
> +				reg = <1>;
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				snps,nr-gpios = <8>;
> +				gpio-ranges = <&pmx0 16 0 8>;

Will operations on local GPIOs 0-7 have a valid pinmux mapping with this
configuration?

The gpio-ranges property uses the format <pinctrl_phandle gpio_base
pinctrl_base count>. By specifying a local gpio_base of 16 for a port with
only 8 pins, it seems gpiolib will fail to configure pins 0-7 correctly.

Should this be inverted to <&pmx0 0 16 8> to map local GPIOs 0-7 to
pinctrl pins 16-23? This appears to be similarly configured for gpio_c and
gpio_d as well.

[ ... ]
> +		pcie_x16: pcie@21000000 {
> +			compatible = "ultrarisc,dp1000-pcie";
> +			reg = <0x0 0x21000000 0x0 0x01000000>,
> +			      <0x0 0x4fff0000 0x0 0x00010000>;
> +			reg-names = "dbi", "config";
> +			ranges = <0x81000000  0x0 0x4fbf0000  0x0 0x4fbf0000  0x0 0x00400000>,

Can legacy PCI endpoints and HBAs that only decode 16-bit I/O addresses
successfully initialize with this mapping?

The ranges property maps the CPU MMIO address 0x4fbf0000 directly to the
PCI bus I/O address 0x4fbf0000 (indicated by the 0x81000000 bitfield).
Would it be better to map the CPU MMIO region to a PCI bus I/O address of 0
instead?

[ ... ]
> +		ethernet: ethernet@38000000 {
> +			compatible = "snps,dwmac", "snps,dwmac-5.10a";

Could the OS prematurely bind to the generic driver before applying
hardware-specific driver quirks with this ordering? Device tree bindings
typically require the compatible array to be ordered from most specific to
least specific.

> +			reg = <0x00 0x38000000 0x00 0x1000000>;
> +			clocks = <&csr_clk>;
> +			clock-names = "stmmaceth";
> +			interrupt-parent = <&plic>;
> +			interrupts = <84>;
> +			interrupt-names = "macirq";
> +			local-mac-address = [ff ff ff ff ff ff];

Will the network driver initialize with an invalid broadcast MAC address if
the bootloader does not dynamically overwrite this value?

Using a broadcast address as a source MAC violates IEEE 802.3 standards,
and upstream network switches may drop packets sent by this interface.
Should this be omitted or zeroed out as a placeholder?

> +			max-speed = <1000>;
> +			phy-mode = "rgmii-id";
> +			snps,txpbl = <8>;
> +			snps,rxpbl = <8>;
> +		};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-ultrarisc-pinctrl-v1-0-bf559589ea8a@ultrarisc.com?part=5