Re: [PATCH v3 5/6] arm64: ambarella: add ARCH_AMBARELLA and CV75 EVK DT

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ARCH_AMBARELLA selects non-existent and architecture-mismatched Kconfig symbols, violating Kconfig dependency rules.
- [Low] The GIC interrupt controller node specifies `#size-cells = <1>` while `#address-cells = <0>`, which is structurally invalid in Devicetree.
--

--- Patch [5]: [PATCH v3 5/6] arm64: ambarella: add ARCH_AMBARELLA and CV75 EVK DT ---
commit cf8659acda73b48006f17101c5f47d5bf1d510a1
Author: Long Zhao <[email protected]>

arm64: ambarella: add ARCH_AMBARELLA and CV75 EVK DT

Enable ARCH_AMBARELLA and add a minimal CV75 SoC / EVK device tree for
console bring-up with the RCT clocks, UART, pinctrl and GPIO banks.

> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index d2acfac730037..166430ddb3c93 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -26,6 +26,15 @@ config ARCH_SUNXI
>  	help
>  	  This enables support for Allwinner sunxi based SoCs like the A64.
>  
> +config ARCH_AMBARELLA
> +	bool "Ambarella SoC family"
> +	select ARM_PSCI
> +	select HAVE_ARM_ARCH_TIMER
> +	select PINCTRL
> +	select PINCTRL_AMB

[Severity: High]
Does PINCTRL_AMB exist in the kernel tree?

Selecting a non-existent config symbol leaves intended features silently
disabled when a user enables the SoC config.

Also, are ARM_PSCI and HAVE_ARM_ARCH_TIMER the correct symbols for arm64?

These appear to be 32-bit ARM-specific symbols. On arm64, it seems PSCI
and the architected timer are managed by ARM_PSCI_FW and ARM_ARCH_TIMER,
which are already selected by default.

> +	help
> +	  This enables support for the ARM64 based Ambarella SoCs like CV75.
> +

[ ... ]

> diff --git a/arch/arm64/boot/dts/ambarella/cv75.dtsi b/arch/arm64/boot/dts/ambarella/cv75.dtsi
> new file mode 100644
> index 0000000000000..994eac6728ce8
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ambarella/cv75.dtsi

[ ... ]

> +		gic: interrupt-controller@fff0101000 {
> +			compatible = "arm,gic-400";
> +			reg = <0xff 0xf0101000 0x0 0x1000>,
> +			      <0xff 0xf0102000 0x0 0x2000>,
> +			      <0xff 0xf0104000 0x0 0x2000>,
> +			      <0xff 0xf0106000 0x0 0x2000>;
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			#address-cells = <0>;
> +			#size-cells = <1>;

[Severity: Low]
Is this #size-cells value correct given that #address-cells is 0?

When a node has an #address-cells value of 0, it does not define an address
space for its children, and its #size-cells property should also be 0.

While this will be ignored at runtime since there are no children, it will
trigger a dtc compiler warning for being structurally invalid.

> +			interrupts = <GIC_PPI 9
> +				      (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH)>;
> +		};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5