[PATCH v4 4/6] phy: rockchip: Add support for Naneng eDP phy
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Fri Oct 30 00:07:20 PDT 2026
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] phy: rockchip: unclocked GRF register access in rockchip_edp_phy_probe()
- [Medium] phy: rockchip: stale lane count in rockchip_edp_phy_verify_config()
- [Low] phy: rockchip: missing select REGMAP_MMIO for PHY_ROCKCHIP_NANENG_EDP
--
commit fa6e3e07850c3a98df33755e8389b184ff441616
Author: Damon Ding <damon.ding at rock-chips.com>
phy: rockchip: Add support for Naneng eDP phy
This commit adds a generic PHY driver for the Rockchip NaNeng eDP
transmitter PHY found on the RK3568 SoC, mapping its register window
directly and supporting 1/2/4 lanes at 1.62 and 2.7 Gbps.
> diff --git a/drivers/phy/rockchip/Kconfig b/drivers/phy/rockchip/Kconfig
> index 14698571b6075..202a6973e20ba 100644
> --- a/drivers/phy/rockchip/Kconfig
> +++ b/drivers/phy/rockchip/Kconfig
> @@ -74,6 +74,16 @@ config PHY_ROCKCHIP_NANENG_COMBO_PHY
> Enable this to support the Rockchip PCIe/USB3.0/SATA/QSGMII
> combo PHY with NaNeng IP block.
>
> +config PHY_ROCKCHIP_NANENG_EDP
> + tristate "Rockchip Naneng eDP Transmitter PHY driver"
> + depends on ARCH_ROCKCHIP && OF
> + select GENERIC_PHY
[Severity: Low]
Since rockchip_edp_phy_probe() in drivers/phy/rockchip/phy-rockchip-naneng-edp.c
calls devm_regmap_init_mmio(), can this fail to link when
CONFIG_PHY_ROCKCHIP_NANENG_EDP is enabled without CONFIG_REGMAP_MMIO (or
with CONFIG_PHY_ROCKCHIP_NANENG_EDP=y and CONFIG_REGMAP_MMIO=m), since
ARCH_ROCKCHIP does not select REGMAP_MMIO? Should PHY_ROCKCHIP_NANENG_EDP
select REGMAP_MMIO (or MFD_SYSCON if using the parent syscon regmap)?
[ ... ]
> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-edp.c b/drivers/phy/rockchip/phy-rockchip-naneng-edp.c
> new file mode 100644
> index 0000000000000..8c247eda1dcf8
> --- /dev/null
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-edp.c
[ ... ]
> +static int rockchip_edp_phy_verify_config(struct rockchip_edp_phy *edpphy,
> + struct phy_configure_opts_dp *dp)
> +{
> + int i;
> +
> + /* If changing link rate was required, verify it's supported. */
> + if (dp->set_rate) {
> + switch (dp->link_rate) {
> + case 1620:
> + case 2700:
> + /* valid bit rate */
> + break;
> + default:
> + return -EINVAL;
> + }
> + }
> +
> + if (dp->set_lanes) {
> + /* Verify lane count. */
> + switch (dp->lanes) {
> + case 1:
> + case 2:
> + case 4:
> + /* valid lane count. */
> + break;
> + default:
> + return -EINVAL;
> + }
> + }
> +
> + /*
> + * If changing voltages is required, check swing and pre-emphasis
> + * levels, per-lane.
> + */
> + if (dp->set_voltages) {
> + /* Lane count verified previously. */
> + for (i = 0; i < edpphy->lanes; i++) {
> + if (dp->voltage[i] > 3 || dp->pre[i] > 3)
> + return -EINVAL;
> +
> + /*
> + * Sum of voltage swing and pre-emphasis levels cannot
> + * exceed 3.
> + */
> + if (dp->voltage[i] + dp->pre[i] > 3)
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
[Severity: Medium]
In rockchip_edp_phy_configure(), rockchip_edp_phy_verify_config() runs
before rockchip_edp_phy_set_lanes() updates edpphy->lanes:
rockchip_edp_phy_configure()
rockchip_edp_phy_verify_config()
if (dp->set_voltages)
for (i = 0; i < edpphy->lanes; i++)
rockchip_edp_phy_set_lanes()
edpphy->lanes = dp->lanes;
rockchip_edp_phy_set_voltages()
for (lane = 0; lane < edpphy->lanes; lane++)
rockchip_edp_phy_set_voltage()
If a caller sets both set_lanes and set_voltages in the same
phy_configure() call when edpphy->lanes is 0 or smaller than dp->lanes,
won't rockchip_edp_phy_verify_config() only validate up to the old
edpphy->lanes count, allowing rockchip_edp_phy_set_voltage() to index
swing_pre_emph_table[dp->voltage[lane]][dp->pre[lane]] out of bounds or
write the -1 sentinel values to the PHY registers for the newly enabled
lanes?
Should rockchip_edp_phy_verify_config() check up to
dp->set_lanes ? dp->lanes : edpphy->lanes instead?
[ ... ]
> +static int rockchip_edp_phy_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct rockchip_edp_phy *edpphy;
> + void __iomem *regs;
> + struct phy *phy;
> + struct phy_provider *phy_provider;
> +
> + edpphy = devm_kzalloc(dev, sizeof(*edpphy), GFP_KERNEL);
> + if (!edpphy)
> + return -ENOMEM;
> +
> + edpphy->dev = dev;
> +
> + 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");
[Severity: High]
In the device tree binding
(Documentation/devicetree/bindings/phy/rockchip,rk3568-edp-phy.yaml), the
APB clock PCLK_EDPPHY_GRF is attached to the parent edp_phy_grf syscon
node, while the child phy at 0 node only has refclk:
edp_phy_grf: syscon at fdcb0000 {
compatible = "rockchip,rk3568-edp-phy-grf", "syscon", "simple-mfd";
reg = <0x0 0xfdcb0000 0x0 0x8000>;
clocks = <&cru PCLK_EDPPHY_GRF>;
...
edp_phy: phy at 0 {
compatible = "rockchip,rk3568-edp-phy";
reg = <0x0 0x100>;
clocks = <&pmucru XIN_OSC0_EDPPHY_G>;
clock-names = "refclk";
#phy-cells = <0>;
};
};
In drivers/clk/rockchip/clk-rk3568.c, PCLK_EDPPHY_GRF is a normal gated
clock and is not marked critical:
GATE(PCLK_EDPPHY_GRF, "pclk_edpphy_grf", "pclk_top", 0,
RK3568_CLKGATE_CON(34), 14, GFLAGS),
Because simple-mfd does not enable the parent node's clocks, and the syscon
driver only attaches and enables a syscon node's clock when a consumer
looks up its regmap via syscon_node_to_regmap(), mapping the register
window directly in rockchip_edp_phy_probe() with devm_regmap_init_mmio()
means PCLK_EDPPHY_GRF is never prepared or enabled (and will be gated by
clk_disable_unused()).
Won't accessing edpphy->regmap in rockchip_edp_phy_power_on(),
rockchip_edp_phy_configure(), and rockchip_edp_phy_power_off() while
PCLK_EDPPHY_GRF is gated cause an APB bus hang or synchronous external
abort?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261030064905.2248999-1-damon.ding@rock-chips.com?part=4
More information about the linux-phy
mailing list