Re: [PATCH v2 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] phy: rockchip: missing set_lanes handling in rockchip_edp_phy_configure()
--

commit 30f8999d3eeb2007dc841979e7a48410043b002b
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 (with the eDP PHY child node) and the eDP
controller device tree nodes for the RK3568 and RK3566 SoCs.

> diff --git a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> index a5832895bd392..3b60e4355ad8c 100644
> --- a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> @@ -403,6 +403,20 @@ 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 0x100>;
> +		clocks = <&cru PCLK_EDPPHY_GRF>;
> +
> +		edp_phy: edp-phy {
> +			compatible = "rockchip,rk3568-edp-phy";
> +			clocks = <&pmucru XIN_OSC0_EDPPHY_G>;
> +			clock-names = "refclk";
> +			#phy-cells = <0>;
> +			status = "disabled";
> +		};
> +	};
> +
>  	pmucru: clock-controller@fdd00000 {
>  		compatible = "rockchip,rk3568-pmucru";
>  		reg = <0x0 0xfdd00000 0x0 0x1000>;
> @@ -879,6 +893,33 @@ hdmi_out: port@1 {
>  		};
>  	};
>  
> +	edp: edp@fe0c0000 {
> +		compatible = "rockchip,rk3568-edp";
> +		reg = <0x0 0xfe0c0000 0x0 0x10000>;
> +		clocks = <&pmucru XIN_OSC0_EDPPHY_G>, <&cru PCLK_EDP_CTRL>, <&cru HCLK_VO>;
> +		clock-names = "dp", "pclk", "hclk";
> +		interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> +		phys = <&edp_phy>;
> +		phy-names = "dp";

[Severity: High]
Can the Analogix eDP controller complete link training when wired to
edp_phy ("rockchip,rk3568-edp-phy") without handling opts->dp.set_lanes in
rockchip_edp_phy_configure()?

When analogix_dp_link_start() runs in
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c, it calls
analogix_dp_set_link_bandwidth(), analogix_dp_set_lane_count(), and
analogix_dp_set_lane_link_training() in
drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c. Each of these functions
zero-initializes a local union phy_configure_opts phy_cfg = {0} and sets
only set_rate, set_lanes, or set_voltages individually:

drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c:
analogix_dp_set_lane_link_training() {
	if (dp->phy) {
		union phy_configure_opts phy_cfg = {0};

		for (lane = 0; lane < dp->link_train.lane_count; lane++) {
			...
			phy_cfg.dp.voltage[lane] = vs;
			phy_cfg.dp.pre[lane] = pe;
		}

		phy_cfg.dp.set_voltages = true;
		ret = phy_configure(dp->phy, &phy_cfg);

However, in drivers/phy/rockchip/phy-rockchip-naneng-edp.c,
rockchip_edp_phy_configure() only checks opts->dp.set_rate and
opts->dp.set_voltages:

drivers/phy/rockchip/phy-rockchip-naneng-edp.c:
rockchip_edp_phy_configure() {
	if (opts->dp.set_rate) {
		ret = rockchip_edp_phy_set_rate(edpphy, &opts->dp);
		if (ret) {
			dev_err(edpphy->dev, "Failed to set rate\n");
			return ret;
		}
	}

	if (opts->dp.set_voltages) {
		ret = rockchip_edp_phy_set_voltages(edpphy, &opts->dp);
		if (ret) {
			dev_err(edpphy->dev, "Failed to set voltages\n");
			return ret;
		}
	}

Because each phy_configure() call passes a fresh struct with only one
action flag set:

1. In analogix_dp_set_link_bandwidth(), phy_cfg.dp.set_rate is true while
phy_cfg.dp.lanes is 0. rockchip_edp_phy_verify_config() skips the lanes
check because dp->set_lanes is false, and rockchip_edp_phy_set_rate() then
evaluates ~GENMASK(dp->lanes - 1, 0) with dp->lanes == 0, underflowing
dp->lanes - 1 when programming EDP_PHY_TX_PD_MASK and EDP_PHY_TX_IDLE_MASK:

drivers/phy/rockchip/phy-rockchip-naneng-edp.c:
rockchip_edp_phy_set_rate() {
	regmap_write(edpphy->grf, EDP_PHY_GRF_CON0,
		     FIELD_PREP_WM16(EDP_PHY_PD_PLL_MASK, 0));
	regmap_write(edpphy->grf, EDP_PHY_GRF_CON0,
		     FIELD_PREP_WM16(EDP_PHY_TX_PD_MASK, ~GENMASK(dp->lanes - 1, 0)));
	...
	regmap_write(edpphy->grf, EDP_PHY_GRF_CON0,
		     FIELD_PREP_WM16(EDP_PHY_TX_IDLE_MASK, ~GENMASK(dp->lanes - 1, 0)));

2. In analogix_dp_set_lane_count(), phy_cfg.dp.set_lanes is true and
phy_cfg.dp.lanes holds the lane count, but rockchip_edp_phy_configure()
ignores opts->dp.set_lanes, neither saving the lane count nor updating the
lane power and idle masks.

3. In analogix_dp_set_lane_link_training(), phy_cfg.dp.set_voltages is true
while phy_cfg.dp.lanes is 0, so rockchip_edp_phy_set_voltages() loops from
0 to dp->lanes (0) and never writes the voltage swing or pre-emphasis
settings:

drivers/phy/rockchip/phy-rockchip-naneng-edp.c:
rockchip_edp_phy_set_voltages() {
	u8 lane;

	for (lane = 0; lane < dp->lanes; lane++)
		rockchip_edp_phy_set_voltage(edpphy, dp, lane);

	return 0;
}

Should the Naneng eDP PHY driver handle opts->dp.set_lanes and cache the
configured lane count in struct rockchip_edp_phy so that separate
set_rate, set_lanes, and set_voltages calls from the Analogix DP controller
work properly?

> +		power-domains = <&power RK3568_PD_VO>;
> +		resets = <&cru SRST_EDP_24M>, <&cru SRST_P_EDP_CTRL>;
> +		reset-names = "dp", "apb";
> +		status = "disabled";
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019102824.3157044-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