Re: [PATCH 22/27] phy: rockchip-naneng-combphy: Add RK3572 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] phy: rockchip-naneng-combphy: writel() clobbers SSC bit in rk3572_combphy_cfg()
--

commit 9ca36a68e2ebd41ee7e126faeae68aae4fa05eae
Author: Cunhao Lu <1579567540@qq.com>

phy: rockchip-naneng-combphy: Add RK3572 support

This commit adds configuration tables, register programming, and device tree
matching support for the RK3572 combo PHY operating in PCIe, USB3, and SATA
modes.

> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> index 7843356a4dd47..e13a78ab57779 100644
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> @@ -1000,6 +1000,204 @@ static const struct rockchip_combphy_cfg rk3568_combphy_cfgs = {
>  	.combphy_cfg	= rk3568_combphy_cfg,
>  };
>  
> +static int rk3572_combphy_cfg(struct rockchip_combphy_priv *priv)
> +{

[ ... ]

> +	switch (rate) {
> +	case 24000000:
> +		rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_clk_24m, true);
> +		if (priv->type == PHY_TYPE_USB3 || priv->type == PHY_TYPE_SATA) {
> +			/* Set ssc_cnt[9:0]=0101111101 & 31.5KHz */
> +			rockchip_combphy_updatel(priv, GENMASK(7, 6), BIT(6), 0xe << 2);
> +
> +			rockchip_combphy_updatel(priv, GENMASK(7, 0), 0x5f, 0xf << 2);
> +		} else if (priv->type == PHY_TYPE_PCIE) {
> +			/* PLL KVCO tuning fine */
> +			rockchip_combphy_updatel(priv, GENMASK(4, 2), 0x4 << 2, 0x20 << 2);

[ ... ]

> +	if (priv->ext_refclk) {
> +		rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_clk_ext, true);
> +		if (priv->type == PHY_TYPE_PCIE && rate == 100000000) {
> +			writel(0x10, priv->mmio + (0x20 << 2));
> +
> +			writel(0x0c, priv->mmio + (0x1b << 2));
> +
> +			/*
> +			 * Set up su_trim: T3_P1 650mv
> +			 * CKRCV termination resistor adjust 11: Hi-Z
> +			 */
> +			writel(0x90, priv->mmio + (0xa << 2));
> +			writel(0x43, priv->mmio + (0xb << 2));
> +			writel(0xb8, priv->mmio + (0xc << 2));
> +			writel(0x56, priv->mmio + (0xd << 2));
> +		}
> +	}
> +
> +	if (priv->enable_ssc) {
> +		rockchip_combphy_updatel(priv, GENMASK(4, 4), BIT(4), 0x7 << 2);
> +		/* PLL control SSC module period should be set */
> +		rockchip_combphy_updatel(priv, GENMASK(5, 5), BIT(5), 0x20 << 2);
> +
> +		if (priv->type == PHY_TYPE_PCIE && rate == 24000000) {
> +			/* xin24m T0_1 650mV */
> +			writel(0x00, priv->mmio + (0x10 << 2));
> +			writel(0x32, priv->mmio + (0x11 << 2));
> +			writel(0x00, priv->mmio + (0x1b << 2));
> +			writel(0x90, priv->mmio + (0x0a << 2));
> +			writel(0x02, priv->mmio + (0x0b << 2));
> +			writel(0x08, priv->mmio + (0x0c << 2));
> +			writel(0x57, priv->mmio + (0x0d << 2));
> +			writel(0x40, priv->mmio + (0x0e << 2));
> +			writel(0x5f, priv->mmio + (0x0f << 2));
> +			writel(0x10, priv->mmio + (0x20 << 2));

[Severity: Medium]
Does this writel(0x10, priv->mmio + (0x20 << 2)) in rk3572_combphy_cfg()
clobber the SSC period control bit that was just set above?

When priv->enable_ssc is true, rk3572_combphy_cfg() sets BIT(5) in register
offset 0x20 << 2 via rockchip_combphy_updatel(). Immediately afterwards, when
priv->type == PHY_TYPE_PCIE and rate == 24000000, this writel() overwrites the
entire register at 0x20 << 2 with 0x10, clearing BIT(5) back to 0.

Should both this write and the writel(0x10, priv->mmio + (0x20 << 2)) in the
priv->ext_refclk block above use rockchip_combphy_updatel() with GENMASK(4, 2)
instead, matching how the 24 MHz and 100 MHz PHY_TYPE_PCIE cases update the
PLL KVCO tuning field earlier in rk3572_combphy_cfg()?

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-add-board-ok3572-c-v1-0-0b0fd85a31ee@qq.com?part=22