Re: [PATCH 2/2] arm64: dts: Add Airoha AN7583 SoC and AN7583 Evaluation Board

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The memory node specifies a physical size of 8GB, which is a typo that will cause a fatal boot panic.
- [Medium] The UART compatible string 'ns16550' will disable hardware FIFOs.
- [Medium] The GIC maintenance interrupt polarity is incorrectly specified as active-low.
- [Low] Missing SoC-specific compatible string for the watchdog timer.
- [Low] Unnecessary address and size cells in the GIC node will trigger compiler warnings.
- [Low] CPU nodes declare cooling capabilities but lack required frequency scaling properties.
--

commit f0ce6065ddf4c46b3e0a82baac122d34553f5f4c
Author: Lorenzo Bianconi <lorenzo@kernel.org>

arm64: dts: Add Airoha AN7583 SoC and AN7583 Evaluation Board

Introduce the Airoha AN7583 SoC's dtsi and the Airoha AN7583 Evaluation
Board's dts file.

> diff --git a/arch/arm64/boot/dts/airoha/an7583-evb.dts b/arch/arm64/boot/dts/airoha/an7583-evb.dts
> new file mode 100644
> index 0000000000000..76b3d94434f63
> --- /dev/null
> +++ b/arch/arm64/boot/dts/airoha/an7583-evb.dts

[ ... ]

> +	memory@80000000 {
> +		device_type = "memory";
> +		reg = <0x0 0x80000000 0x2 0x00000000>;
> +	};

Because #size-cells = <2>, the size array <0x2 0x00000000> sets the upper
32 bits to 0x2, which evaluates to an enormous 8GB memory range.

Is this a typo for 512MB (<0x0 0x20000000>) or 2GB (<0x0 0x80000000>)?

While an identical value exists in the predecessor board's DT, it was
safely masked there by a restrictive linux,usable-memory-range property
in the chosen node.

Since this board lacks that protective limit, will the kernel attempt to
map and access the full 8GB of memory and trigger unbacked physical
memory accesses, resulting in a fatal bus fault during early boot?

> diff --git a/arch/arm64/boot/dts/airoha/an7583.dtsi b/arch/arm64/boot/dts/airoha/an7583.dtsi
> new file mode 100644
> index 0000000000000..64b2c7533aff6
> --- /dev/null
> +++ b/arch/arm64/boot/dts/airoha/an7583.dtsi

[ ... ]

> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53";
> +			reg = <0x0>;
> +			enable-method = "psci";
> +			next-level-cache = <&l2>;
> +			#cooling-cells = <2>;
> +		};

Are required frequency scaling properties missing here?

Both CPU nodes define #cooling-cells, but the cpufreq_cooling subsystem
requires properties like clocks or operating-points-v2 to bind. Without
them, is this cooling declaration functionally dead code that generates
validation warnings?

[ ... ]

> +		gic: interrupt-controller@9000000 {
> +			compatible = "arm,gic-v3";
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;

Do we need these address and size cells here? According to device tree
compiler rules, specifying them without any children or a ranges property
causes an avoid_unnecessary_addr_size warning.

> +			reg = <0x0 0x09000000 0x0 0x20000>,
> +			      <0x0 0x09080000 0x0 0x80000>,
> +			      <0x0 0x09400000 0x0 0x2000>,
> +			      <0x0 0x09500000 0x0 0x2000>,
> +			      <0x0 0x09600000 0x0 0x20000>;
> +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;

Should this maintenance interrupt be active-high?

The ARM Generic Interrupt Controller architecture specification explicitly
mandates that the GIC maintenance interrupt is an active-high,
level-sensitive interrupt (IRQ_TYPE_LEVEL_HIGH).

Incorrect polarity might prevent it from firing properly and break
hardware virtualization support.

> +		};
> +
> +		uart1: serial@1fbf0000 {
> +			compatible = "ns16550";

Could this compatible string unintentionally disable hardware FIFOs?

In the Linux 8250 serial driver, ns16550 restricts the fifo_size to 1
byte due to known hardware bugs in the original 16550 chip. Since modern
UART IPs include working FIFOs, should this be declared as ns16550a to
avoid excessive interrupt load?

> +			reg = <0x0 0x1fbf0000 0x0 0x30>;
> +			reg-io-width = <4>;
> +			reg-shift = <2>;
> +			interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> +			clock-frequency = <1843200>;
> +		};
> +
> +		watchdog@1fbf0100 {
> +			compatible = "airoha,en7581-wdt";

Would it be better to include the SoC-specific compatible string alongside
the fallback?

The dt-binding schema typically dictates using a string like
"airoha,an7583-wdt", "airoha,en7581-wdt" to accurately describe the
specific SoC and ensure any future platform-specific driver quirks can be
cleanly applied.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509-airoha-7583-v1-0-e5ad085b258e@kernel.org?part=2