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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] riscv: dts: starfive: CMA alloc-ranges in jh7110-common.dtsi
  breaks 1 GiB RAM
- [Low] riscv: dts: starfive: missing status disabled on jh7110.dtsi
  display nodes
- [Low] riscv: dts: starfive: duplicate 24 MHz xin24m clock node in
  jh7110.dtsi
--

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

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

This commit describes the JH7110 video output subsystem, DC8200 display
controller, HDMI controller and PHY in the SoC device tree, and enables
the display pipeline with a 512 MiB default CMA pool in the common board
device tree.

> diff --git a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> index 64de468f2c317..bfc7ae1282234 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>;
> +			linux,cma-default;
> +		};
> +	};

[Severity: High]
Does this alloc-ranges setting cause linux,cma reservation to fail on 1 GiB
JH7110 boards, or whenever an earlier reservation overlaps 0x70000000..
0x90000000?

jh7110-common.dtsi is also included by 1 GiB boards such as Milk-V Mars and
Mars CM, where physical RAM at 0x40000000..0x80000000 only covers 256 MiB of
the 0x70000000..0x90000000 window, so __reserved_mem_alloc_size() fails with
-ENOMEM and leaves the system without a default CMA pool. Because the window
size equals the 512 MiB allocation size with no slack, any bootloader or
initrd reservation in 0x70000000..0x90000000 also causes CMA allocation to
fail on larger boards even though DC8200 uses a 32-bit DMA mask and can
address any RAM below 0x100000000.

[ ... ]

> @@ -644,3 +757,7 @@ &U74_3 {
>  &U74_4 {
>  	cpu-supply = <&vdd_cpu>;
>  };
> +
> +&voutcrg {
> +	status = "okay";
> +};
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index 82ea63f715b0e..ac10d79770f24 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi

[ ... ]

> @@ -352,6 +346,13 @@ tdm_ext: tdm-ext-clock {
>  		#clock-cells = <0>;
>  	};
>  
> +	xin24m: xin24m-clock {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <24000000>;
> +		clock-output-names = "xin24m";
> +	};

[Severity: Low]
Can hdmi_phy use the existing <&osc> node in jh7110.dtsi instead of adding a
duplicate 24 MHz xin24m fixed-clock node?

jh7110.dtsi already defines the 24 MHz reference crystal as osc: oscillator
(with its frequency set in jh7110-common.dtsi), and neither the
starfive,jh7110-inno-hdmi-phy binding nor starfive_hdmi_phy_probe() requires
the clock to be named xin24m.

> @@ -1262,22 +1263,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;
> +			#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";
> +				reg = <0x0 0x29400000 0x0 0x2800>;
> +				clocks = <&voutcrg JH7110_VOUTCLK_DC8200_CORE>,
> +					 <&voutcrg JH7110_VOUTCLK_DC8200_AXI>,
> +					 <&voutcrg JH7110_VOUTCLK_DC8200_AHB>,
> +					 <&voutcrg JH7110_VOUTCLK_DC8200_PIX0>,
> +					 <&voutcrg JH7110_VOUTCLK_DC8200_PIX1>;
> +				clock-names = "core", "axi", "ahb", "pix0", "pix1";
> +				assigned-clocks = <&voutcrg JH7110_VOUTCLK_DC8200_PIX0>,
> +						  <&voutcrg JH7110_VOUTCLK_DC8200_PIX1>;
> +				assigned-clock-parents = <&hdmi_phy>, <&hdmi_phy>;
> +				interrupts = <95>;
> +				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";
> +				reg = <0x0 0x29590000 0x0 0x4000>;
> +
> +				hdmi_controller: controller {
> +					compatible =
> +						"starfive,jh7110-inno-hdmi-controller";
> +					clocks = <&voutcrg JH7110_VOUTCLK_HDMI_TX_SYS>,
> +						 <&voutcrg JH7110_VOUTCLK_HDMI_TX_MCLK>,
> +						 <&voutcrg JH7110_VOUTCLK_HDMI_TX_BCLK>,
> +						 <&hdmi_phy>;
> +					clock-names = "pclk", "mclk", "bclk", "pixel";
> +					interrupts = <99>;
> +					phys = <&hdmi_phy>;
> +					resets = <&voutcrg JH7110_VOUTRST_HDMI_TX_HDMI>;
> +					starfive,vout-syscon = <&vout_syscon>;
> +				};

[Severity: Low]
Should dc8200 and hdmi_controller have status = "disabled" in jh7110.dtsi,
and should the &voutcrg override in jh7110-common.dtsi be dropped?

Both dc8200 and hdmi_controller require board-specific port graphs and
pinctrl properties that are only populated in jh7110-common.dtsi (which
explicitly sets status = "okay" on both nodes), so leaving them enabled by
default in jh7110.dtsi leaves them incomplete for any board not using
jh7110-common.dtsi. Conversely, voutcrg is never disabled in jh7110.dtsi, so
the &voutcrg block setting status = "okay" in jh7110-common.dtsi has no
effect.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-jh7110-clean-send-v7-0-68f7ea9913db@samsung.com?part=18