Re: [PATCH v3 18/19] riscv: dts: starfive: jh7110: Update DT for display subsystem
From: sashiko-bot@kernel.org
Date: 2026-09-04 14:03:22
Also in:
dri-devel, linux-clk, linux-devicetree
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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 */ + };
[ ... ]
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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>;[ ... ]
quoted hunk ↗ jump to hunk
@@ -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 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy