Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Cyclic DT dependency between DP controllers, their child PHY nodes, and the APMU syscon will cause a fw_devlink warning and a potential -EPROBE_DEFER deadlock. - [Medium] Fragile array override of `syscon_apmu` clocks creates a severe maintenance hazard. --
--- Patch [14]: [PATCH 14/17] riscv: dts: spacemit: k3: add display nodes --- commit c12c6ab932374f52bfc3ce3d04640d8193a9bfea Author: Cody Kang <[email protected]> riscv: dts: spacemit: k3: add display nodes Add the SoC-level display nodes: the two Saturn DPUs, the two DP/eDP controllers with their PHY children. > diff --git a/arch/riscv/boot/dts/spacemit/k3-display.dtsi > b/arch/riscv/boot/dts/spacemit/k3-display.dtsi > new file mode 100644 > index 0000000000000..4892ee6acda22 > --- /dev/null > +++ b/arch/riscv/boot/dts/spacemit/k3-display.dtsi [ ... ] > + edp0: edp@cac84000 { > + compatible = "spacemit,k3-inno-dp"; > + reg = <0x0 0xcac84000 0x0 0x4000>; > + interrupts = <132 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&syscon_apmu CLK_APMU_EDP0_PXCLK>, <&edp0_phy>; [Severity: High] Does this create a cyclical dependency that could cause a fw_devlink warning or a permanent probe deadlock? edp0 depends on syscon_apmu here, while syscon_apmu depends on the edp0_phy child node below. Since edp0_phy is a child of edp0, it won't be populated until edp0 probes. If fw_devlink breaks the cycle by dropping the edp0 -> syscon_apmu link, edp0 will attempt to probe first, fail to get its clock, and defer forever because it will not proceed to populate the child PHY. > + clock-names = "pxclk", "pll"; > + resets = <&syscon_apmu RESET_APMU_EDP0>; > + phys = <&edp0_phy>; > + phy-names = "dp"; > + status = "disabled"; > + > + edp0_phy: phy { > + compatible = "spacemit,k3-inno-dp-phy"; > + #phy-cells = <0>; > + #clock-cells = <0>; > + clock-output-names = "edp0_pll_pxclk"; > + }; [ ... ] > +/* > + * Feed the PHY PLL pixel clocks back in as the APMU mux parents. This lives > + * here, not in k3.dtsi: the PHYs only exist where this file is included. > + */ > +&syscon_apmu { > + clocks = <&osc_32k>, <&vctcxo_1m>, <&vctcxo_3m>, <&vctcxo_24m>, > + <&edp0_phy>, <&dp1_phy>; > + clock-names = "osc", "vctcxo_1m", "vctcxo_3m", "vctcxo_24m", > + "edp0_pll_pxclk", "edp1_pll_pxclk"; [Severity: Medium] Will overriding the clocks property completely drop any future additions made to the syscon_apmu node in the base k3.dtsi? Device tree doesn't support appending to array properties, so if any new clocks are added to the base syscon_apmu node later, this file will silently override them and break other subsystems relying on those clocks. > +}; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
