Re: [PATCH v3 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 issue(s) to consider:
- [High] The HDMI HPD (Hot Plug Detect) pin is incorrectly configured to drive a logic HIGH output, causing electrical contention and breaking hotplug detection.
- [Medium] Hardware peripheral nodes in the SoC dtsi lack 'status = "disabled"', which will break DT validation for custom boards.
- [Low] The device tree introduces a redundant `xin24m` fixed-clock which duplicates the existing `osc` clock.
--

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

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

Activate the display subsystem by describing the video output subsystem,
which the documentation calls dom_vout_top, as a bus node covering the
whole 0x29400000 region.

> diff --git a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> --- a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
[ ... ]
> @@ -351,6 +430,40 @@ &syscrg {
>  };
>  
>  &sysgpio {
> +	hdmi_pins: hdmi-0 {
> +		hdmi-cec-pins {
> +			pinmux = <GPIOMUX(14, GPOUT_SYS_HDMI_CEC_SDA,
> +					      GPOEN_SYS_HDMI_CEC_SDA,
> +					      GPI_SYS_HDMI_CEC_SDA)>;
> +			input-enable;
> +			bias-pull-up;
> +		};
> +
> +		hdmi-hpd-pins {
> +			pinmux = <GPIOMUX(15, GPOUT_HIGH,
> +					      GPOEN_ENABLE,
> +					      GPI_SYS_HDMI_HPD)>;

[Severity: High]
Does this configuration incorrectly drive a logic HIGH output on the HPD pin?

The HDMI HPD (Hot Plug Detect) pin is structurally an input driven by the
external HDMI sink. By statically driving the output driver HIGH from the SoC
side (using GPOEN_ENABLE and GPOUT_HIGH), the physical line is held high
regardless of what the monitor tries to drive.

Can this break hotplug detection logic and cause electrical contention if the
monitor pulls it low? Standard input pins usually use GPOEN_DISABLE to prevent
driving the line.

> +			input-enable;
> +			bias-disable; /* external pull-up */
> +		};
[ ... ]
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> --- 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]
Is this xin24m fixed-clock redundant?

The board's 24MHz oscillator is typically already modeled via an existing osc
node which receives its frequency from board-specific common files. 

Does adding xin24m here unnecessarily duplicate the existing oscillator
node rather than reusing it?

>  	soc {
>  		compatible = "simple-bus";
>  		interrupt-parent = <&plic>;
[ ... ]
> @@ -1199,22 +1200,81 @@ csi_phy: phy@19820000 {
>  			#phy-cells = <0>;
>  		};
>  
> +		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>;

[Severity: Medium]
Should this hardware peripheral node include status = "disabled";?

Hardware peripheral nodes in the SoC dtsi are generally disabled by default.
Because the dc8200 node requires board-specific properties (like ports) that
are only provided in jh7110-common.dtsi, leaving it active by default here
can cause DT schema validation failures.

Can this break dtbs_check or cause driver probe crashes on custom boards that
include jh7110.dtsi but do not wire up the display components?

> +				clocks = <&voutcrg JH7110_VOUTCLK_DC8200_CORE>,
[ ... ]
> +				reset-names = "core", "axi", "ahb";
> +			};
> +
> +			hdmi_subsystem: hdmi@29590000 {
> +				compatible = "starfive,jh7110-hdmi-subsystem";
> +				reg = <0x0 0x29590000 0x0 0x4000>;

[Severity: Medium]
Should the hdmi_subsystem node also include status = "disabled";?

Similar to the dc8200 node, leaving this active by default can lead to
validation errors if it is left unconfigured by board files.

> +
> +				hdmi_controller: controller {
[ ... ]
> +				hdmi_phy: phy {
> +					compatible = "starfive,jh7110-inno-hdmi-phy";
> +					clocks = <&xin24m>;

[Severity: Low]
Should this consume the standard <&osc> clock instead of <&xin24m>?

If the xin24m node is redundant as mentioned above, this would likely need
to be updated to point to the main oscillator node.

> +					#clock-cells = <0>;
> +					#phy-cells = <0>;
> +				};
> +			};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-jh7110-clean-send-v3-0-484f9ae72715@samsung.com?part=18