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 <[email protected]> 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/[email protected]?part=6
