Re: [PATCH v4 19/20] riscv: dts: starfive: jh7110: Update DT for display subsystem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dts: CMA allocation failure on 1GB boards
- [Medium] dts: missing 'status = "disabled"' in SoC DTSI for display IPs
- [Low] dts: redundant reg property on display-subsystem bus node
--

commit d9fbb93d7964da0ec9e48356030bc459050f3218
Author: Michal Wilczynski <m.wilczynski@samsung.com>

riscv: dts: starfive: jh7110: Update DT for display subsystem

This commit activates the display subsystem on the JH7110 SoC by describing
the video output subsystem, HDMI block, and voutcrg in the device tree. It
restructures the HDMI node into a container with phy and controller children
to break a circular clock dependency. Additionally, the CMA pool is expanded
and constrained to memory addressable by the display controller.

> diff --git a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> index a7a1c09a2c907..a4c3daf712a63 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> @@ -33,6 +33,27 @@ memory@40000000 {
>  		bootph-pre-ram;
>  	};
>  
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		/*
> +		 * The display controller allocates its framebuffers from CMA,
> +		 * and the default pool is too small for them. Keep the pool
> +		 * inside the range the controller can address; it fits in the
> +		 * memory every VisionFive 2 variant has.
> +		 */
> +		linux,cma {
> +			compatible = "shared-dma-pool";
> +			reusable;
> +			size = <0x0 0x20000000>;
> +			alignment = <0x0 0x1000>;
> +			alloc-ranges = <0x0 0x70000000 0x0 0x20000000>;

[Severity: High]
Will this unconditionally fail to allocate the CMA pool on 1GB boards?

On boards like the 1GB Milk-V Mars, physical RAM starts at 0x40000000 and 
ends at 0x80000000. With alloc-ranges starting at 0x70000000, there is only
256MB of physical memory available within the requested bounds, making a
512MB allocation mathematically impossible.

> +			linux,cma-default;
> +		};
> +	};
> +

[ ... ]

> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index 9c3e4598747eb..bc2470963520e 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi

[ ... ]

> @@ -1199,22 +1200,84 @@ csi_phy: phy@19820000 {
>  			#phy-cells = <0>;
>  		};
>  
> -		voutcrg: clock-controller@295c0000 {
> -			compatible = "starfive,jh7110-voutcrg";
> -			reg = <0x0 0x295c0000 0x0 0x10000>;
> -			clocks = <&syscrg JH7110_SYSCLK_VOUT_SRC>,
> -				 <&syscrg JH7110_SYSCLK_VOUT_TOP_AHB>,
> -				 <&syscrg JH7110_SYSCLK_VOUT_TOP_AXI>,
> -				 <&syscrg JH7110_SYSCLK_VOUT_TOP_HDMITX0_MCLK>,
> -				 <&syscrg JH7110_SYSCLK_I2STX0_BCLK>,
> -				 <&hdmitx0_pixelclk>;
> -			clock-names = "vout_src", "vout_top_ahb",
> -				      "vout_top_axi", "vout_top_hdmitx0_mclk",
> -				      "i2stx0_bclk", "hdmitx0_pixelclk";
> -			resets = <&syscrg JH7110_SYSRST_VOUT_TOP_SRC>;
> -			#clock-cells = <1>;
> -			#reset-cells = <1>;
> +		vout_subsystem: display-subsystem@29400000 {
> +			compatible = "starfive,jh7110-vout-subsystem";
> +			reg = <0x0 0x29400000 0x0 0x200000>;
> +			ranges;

[Severity: Low]
Is this reg property redundant on the bus node?

It assigns a memory region to a bus node that does not own configuration
registers, simply to represent the aggregated bounds of its children. The
dc8200 child claims the exact same starting physical registers.

> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			clocks = <&syscrg JH7110_SYSCLK_NOC_BUS_DISP_AXI>;
> +			dma-noncoherent;
>  			power-domains = <&pwrc JH7110_PD_VOUT>;
> +			resets = <&syscrg JH7110_SYSRST_NOC_BUS_DISP_AXI>;
> +
> +			dc8200: display@29400000 {
> +				compatible = "starfive,jh7110-dc8200", "verisilicon,dc";

[Severity: Medium]
Should this SoC-level IP definition include status = "disabled"?

Custom boards that include jh7110.dtsi but do not include jh7110-common.dtsi
will attempt to probe this IP by default. Headless boards lacking physical
HDMI connectors and necessary board-level pinmuxing may experience probe
errors or unwanted resource consumption.

> +				reg = <0x0 0x29400000 0x0 0x2800>;
> +				clocks = <&voutcrg JH7110_VOUTCLK_DC8200_CORE>,
> +					 <&voutcrg JH7110_VOUTCLK_DC8200_AXI>,

[ ... ]

> +				resets = <&voutcrg JH7110_VOUTRST_DC8200_CORE>,
> +					 <&voutcrg JH7110_VOUTRST_DC8200_AXI>,
> +					 <&voutcrg JH7110_VOUTRST_DC8200_AHB>;
> +				reset-names = "core", "axi", "ahb";
> +			};
> +
> +			hdmi_subsystem: hdmi@29590000 {
> +				compatible = "starfive,jh7110-hdmi-subsystem";

[Severity: Medium]
Should the hdmi_subsystem IP definition also be disabled by default at the
SoC level?

Like the dc8200 node above, enabling this by default in the SoC devicetree
may cause unintended probe attempts on boards that do not utilize the display
subsystem.

> +				reg = <0x0 0x29590000 0x0 0x4000>;
> +
> +				hdmi_controller: controller {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915-jh7110-clean-send-v4-0-f0e4fd6f2cc8@samsung.com?part=19