Re: [PATCH v4 4/6] phy: rockchip: Add support for Naneng eDP phy

[sashiko-bot@kernel.org]

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@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@0 node only has refclk:

      edp_phy_grf: syscon@fdcb0000 {
        compatible = "rockchip,rk3568-edp-phy-grf", "syscon", "simple-mfd";
        reg = <0x0 0xfdcb0000 0x0 0x8000>;
        clocks = <&cru PCLK_EDPPHY_GRF>;
        ...
        edp_phy: phy@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