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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 5173 bytes --]

> 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?

ack, I will fix it in v2.

> 
> > 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?

ack, I will fix it in v2.

> 
> [ ... ]
> 
> > +		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.

ack, I will fix it in v2.

> 
> > +			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.

ack, I will fix it in v2.

> 
> > +		};
> > +
> > +		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?

ack, I will fix it in v2.

> 
> > +			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.

ack, I will fix it in v2.

Regards,
Lorenzo

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]