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