Re: [PATCH v4 6/6] arm64: dts: rockchip: rk356x: Add eDP controller and phy nodes

[sashiko-bot@kernel.org]

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 <damon.ding@rock-chips.com>

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/20261030064905.2248999-1-damon.ding@rock-chips.com?part=6

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy