Re: [PATCH v2 14/15] riscv: dts: starfive: jh7110: Update DT for display subsystem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Hardcoded exact physical address range for CMA reservation restricts memory allocation and breaks smaller RAM variants.
- [High] The commit claims genpd powers the PD_VOUT domain on demand, but this is functionally impossible due to missing PM runtime integration in the HDMI subsystem driver.
- [Low] Duplicate 24MHz fixed-clock node introduced for `hdmi_phy`.
- [Low] The `dc8200` node provides only `"verisilicon,dc"` in its `compatible` string, which violates the `verisilicon,dc.yaml` binding schema.
--

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

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

This commit activates the display subsystem by adding the dc8200, hdmi, and
voutcrg nodes as direct soc children. It models the monolithic hdmi node as
a container with hdmi_phy and hdmi_controller children. It also configures
clock, DMA properties, and CMA reservations.

> diff --git a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> index a7a1c09a2c907..7aadc35f4a6aa 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> @@ -33,6 +33,25 @@ memory@40000000 {
>  		bootph-pre-ram;
>  	};
>  
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		/* vout applies for space from this CMA
> +		 * Without this CMA reservation,
> +		 * vout may not work properly.
> +		 */
> +		linux,cma {
> +			compatible = "shared-dma-pool";
> +			reusable;
> +			size = <0x0 0x20000000>;
> +			alignment = <0x0 0x1000>;
> +			alloc-ranges = <0x0 0x70000000 0x0 0x20000000>;
> +			linux,cma-default;

[Severity: High]
Could this alloc-ranges configuration break boot on systems with less
memory or conflicting bootloader placements?

The property alloc-ranges = <0x0 0x70000000 0x0 0x20000000> combined with
size = <0x0 0x20000000> forces the 512MB CMA pool exactly at physical address
0x70000000.

On a board with only 1GB of RAM (ranging from 0x40000000 to 0x80000000),
the top 256MB of this requested region falls completely outside of physical
memory, which would guarantee an allocation failure.

Additionally, if a bootloader dynamically places the DTB or initrd anywhere
within this narrow 512MB slice, CMA reservation will conflict and silently
fail.

> +		};
> +	};
> +
[ ... ]
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index 9c3e4598747eb..024e33b0e97cb 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 {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <24000000>;
> +		clock-output-names = "xin24m";
> +	};
> +

[Severity: Low]
Does this introduce a redundant clock definition?

The jh7110.dtsi and jh7110-common.dtsi files already define an osc clock
node representing the 24MHz physical oscillator on the board. Introducing a
new xin24m fixed-clock node with the exact same 24MHz frequency seems
redundant and creates a hardware modeling flaw in the Device Tree.

Should the hdmi_phy node reference the existing <&osc> clock instead?

>  	soc {
>  		compatible = "simple-bus";
>  		interrupt-parent = <&plic>;
> @@ -1199,22 +1200,92 @@ csi_phy: phy@19820000 {
>  			#phy-cells = <0>;
>  		};
>  
> +		dc8200: display@29400000 {
> +			compatible = "verisilicon,dc";

[Severity: Low]
Is this missing the SoC-specific compatible string required by the binding
schema?

The verisilicon,dc.yaml schema requires the compatible property to be an
array of two items: an SoC-specific string (e.g., thead,th1520-dc8200 or
starfive,jh7110-dc8200) followed by verisilicon,dc.

Specifying only compatible = "verisilicon,dc" violates the standard rule for
IP block instances and will cause dtbs_check validation to fail.

> +			reg = <0x0 0x29400000 0x0 0x2800>;
[ ... ]
> +		hdmi_subsystem: hdmi@29590000 {
> +			compatible = "starfive,jh7110-hdmi-subsystem";
> +			reg = <0x0 0x29590000 0x0 0x4000>;
> +
> +			/*
> +			 * This MFD's PHY child is the first device in the vout
> +			 * region to touch registers, so it owns the NoC display
> +			 * bus clock + reset that gate access to the region.
> +			 * PD_VOUT is powered on by genpd via power-domains.
> +			 */
> +			power-domains = <&pwrc JH7110_PD_VOUT>;

[Severity: High]
Is the claim that genpd powers this domain on demand functionally possible
without PM runtime integration in the HDMI subsystem driver?

The commit message states "genpd powers the domain on demand", and the
hdmi_subsystem node relies on this for PD_VOUT. However, the driver in
drivers/soc/starfive/jh7110-hdmi-subsystem.c does not appear to call
pm_runtime_enable().

Because the container has PM runtime disabled, any pm_runtime_get_sync()
calls from its children will return -EACCES and fail to propagate to genpd.

The hardware appears to only work because the voutcrg clock controller
driver unconditionally calls pm_runtime_resume_and_get() in its probe
function without releasing it. This forcefully pins the PD_VOUT domain ON
indefinitely, which circumvents the on-demand power management expectation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-jh7110-clean-send-v2-0-331680c8b9d1@samsung.com?part=14

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy