Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Shared bus clocks and resets are incorrectly assigned to the child controller instead of the parent, which will cause a system bus fault when the PHY child accesses the shared registers.
- [Medium] Splitting a monolithic hardware block into an MFD purely to resolve a software probe ordering issue violates core Device Tree design principles.
- [Medium] The binding requires a bespoke MFD driver for a simple shared register space by omitting standard fallback compatible strings.
--
commit c934e468899df3820b545d81d3b4b4a9ae4b2097
Author: Michal Wilczynski [off-list ref]
dt-bindings: mfd: Add starfive,jh7110-hdmi-subsystem
Adds a device tree binding for the StarFive JH7110 HDMI subsystem.
The binding creates a multi-function device parent with phy and controller
children to resolve a circular clock dependency.
quoted hunk ↗ jump to hunk
diff --git a/Documentation/devicetree/bindings/mfd/starfive,jh7110-hdmi-subsystem.yaml b/Documentation/devicetree/bindings/mfd/starfive,jh7110-hdmi-subsystem.yaml
new file mode 100644
index 0000000000000..bd35f191835a6
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/starfive,jh7110-hdmi-subsystem.yaml
[ ... ]
quoted hunk ↗ jump to hunk
+description:
+ The StarFive JH7110 HDMI block is a monolithic IP containing both
+ the digital controller logic and the analog PHY logic in a single
+ register space. It sits inside the video output subsystem, which owns
[Severity: Medium]
Since the hardware is a single monolithic IP block in a single register
space, does splitting it into a multi-function device parent and child nodes
conflict with the core device tree principle of describing hardware topology
rather than software abstraction limitations?
quoted hunk ↗ jump to hunk
+ the NoC bus clock and reset gating this region and the power domain it
+ belongs to.
+
+properties:
+ compatible:
+ const: starfive,jh7110-hdmi-subsystem
[Severity: Medium]
Does limiting the compatible property strictly to this string without allowing
standard fallbacks like "simple-mfd" force the creation of a bespoke driver
for a simple shared register space?
quoted hunk ↗ jump to hunk
[ ... ]
quoted hunk ↗ jump to hunk
+required:
+ - compatible
+ - reg
+ - phy
+ - controller
+
+additionalProperties: false
[Severity: High]
With this property set to false, it is impossible for the parent node to
request clocks or resets. In the example below, the module system clock
(JH7110_VOUTCLK_HDMI_TX_SYS) is assigned to the controller child node.
Because the controller depends on the phy child for its pixel clock, the phy
must probe first. When the phy probes and attempts read-modify-write
operations on the shared regmap, will this cause a system bus fault and hard
CPU lockup because the module system clock is not yet enabled by the
controller?
quoted hunk ↗ jump to hunk
+
+examples:
+ - |
+ #include <dt-bindings/clock/starfive,jh7110-crg.h>
+ #include <dt-bindings/reset/starfive,jh7110-crg.h>
+
+ soc {
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ hdmi@29590000 {
+ compatible = "starfive,jh7110-hdmi-subsystem";
+ reg = <0x29590000 0x4000>;
+
+ controller {
+ compatible = "starfive,jh7110-inno-hdmi-controller";
+ clocks = <&voutcrg JH7110_VOUTCLK_HDMI_TX_SYS>,
[Severity: High]
Can this clock assignment lead to the system bus fault mentioned above? It
seems the parent node should be responsible for enabling the bus clock before
the children probe.
quoted hunk ↗ jump to hunk
+ <&voutcrg JH7110_VOUTCLK_HDMI_TX_MCLK>,
+ <&voutcrg JH7110_VOUTCLK_HDMI_TX_BCLK>,
+ <&hdmi_phy>;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-jh7110-clean-send-v3-0-484f9ae72715@samsung.com?part=3
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy