Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] arm64: dts: rockchip: rk356x: PCLK_EDPPHY_GRF never enabled for edp_phy --
commit 4fc93be7caab95400abe782b268ff22b9c12b905 Author: Damon Ding <[email protected]> arm64: dts: rockchip: rk356x: Add eDP controller and phy nodes This commit adds the eDP PHY GRF syscon node, its child eDP PHY node, and the eDP controller node to the RK3566/RK3568 base device tree. > diff --git a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi > b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi > index a5832895bd392..7a5168548e55a 100644 > --- a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi > @@ -403,6 +403,24 @@ usb2phy1_grf: syscon@fdca8000 { > reg = <0x0 0xfdca8000 0x0 0x8000>; > }; > > + edp_phy_grf: syscon@fdcb0000 { > + compatible = "rockchip,rk3568-edp-phy-grf", "syscon", > "simple-mfd"; > + reg = <0x0 0xfdcb0000 0x0 0x8000>; > + clocks = <&cru PCLK_EDPPHY_GRF>; > + ranges = <0x0 0x0 0xfdcb0000 0x100>; > + #address-cells = <1>; > + #size-cells = <1>; > + > + edp_phy: phy@0 { > + compatible = "rockchip,rk3568-edp-phy"; > + reg = <0x0 0x100>; > + clocks = <&pmucru XIN_OSC0_EDPPHY_G>; > + clock-names = "refclk"; > + #phy-cells = <0>; > + status = "disabled"; > + }; > + }; [Severity: High] With PCLK_EDPPHY_GRF placed on the parent edp_phy_grf node while edp_phy maps its register window directly, will PCLK_EDPPHY_GRF ever get enabled before the PHY driver accesses the registers at 0xfdcb0000? In drivers/phy/rockchip/phy-rockchip-naneng-edp.c, rockchip_edp_phy_probe() maps the child reg window directly and creates its own MMIO regmap rather than looking up the parent syscon regmap, and only gets refclk: drivers/phy/rockchip/phy-rockchip-naneng-edp.c:rockchip_edp_phy_probe() { ... regs = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(regs)) return PTR_ERR(regs); edpphy->regmap = devm_regmap_init_mmio(dev, regs, &rockchip_edp_phy_regmap_config); if (IS_ERR(edpphy->regmap)) return dev_err_probe(dev, PTR_ERR(edpphy->regmap), "Failed to init regmap\n"); edpphy->refclk = devm_clk_get(dev, "refclk"); ... } Because no consumer calls syscon_node_to_regmap() on edp_phy_grf (and even if one did, syscon only attaches the clock to its own regmap), and simple-pm-bus treats simple-mfd as ONLY_BUS without managing its clocks, nothing claims or enables PCLK_EDPPHY_GRF. In drivers/clk/rockchip/clk-rk3568.c, PCLK_EDPPHY_GRF is a gated clock that is not marked critical: GATE(PCLK_EDPPHY_GRF, "pclk_edpphy_grf", "pclk_top", 0, RK3568_CLKGATE_CON(34), 14, GFLAGS), Wouldn't clk_disable_unused() gate PCLK_EDPPHY_GRF off, causing unclocked MMIO accesses in rockchip_edp_phy_configure() and rockchip_edp_phy_power_on() when an RK3566/RK3568 board enables edp and edp_phy? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
