Re: [PATCH 2/2] phy: rockchip-naneng-combo: Support rk3576

["Heiko Stübner" <heiko@sntech.de>]

Hi Shawn,

Am Dienstag, 15. Oktober 2024, 10:39:28 CEST schrieb Shawn Lin:
> 在 2024/10/15 14:15, Heiko Stübner 写道:
> > Am Dienstag, 15. Oktober 2024, 03:33:51 CEST schrieb Frank Wang:
> >> From: Kever Yang <kever.yang@rock-chips.com>
> >>
> 
> ...
> 
> >> +			writel(0x02, priv->mmio + (0xb << 2));
> >> +			writel(0x57, priv->mmio + (0xd << 2));
> >> +
> >> +			writel(0x5f, priv->mmio + (0xf << 2));
> > 
> > This does includes both the value as well as the register addresses,
> > because a hex-value with a bit shift makes that even less readable.
> > 
> 
> Actually, it's more readable when we need to debug, IMO. Because the
> PHY document provided is listing the registers just like what the patch
> did(0xb/0xd/0xf). So for example, we could easily find 0xb in the 
> document, which refers to su_trim[15:8].

Documentation-wise the rk3576 has gotten worse, compared to the
rk3588 and before.  I guess you're refering to a document that is not
part of the trm-part1 pdf I guess.

So I guess using register numbers somewhat matching the sparse
document you have can be fine, but I guess try to use constants where
possible.

The block above though at least needs a comment about what is
happening there.


Heiko

> >> +		}
> >> +		break;
> >> +	case REF_CLOCK_25MHz:
> >> +		rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_clk_25m, true);
> >> +		break;
> >> +	case REF_CLOCK_100MHz:
> >> +		rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_clk_100m, true);
> >> +		if (priv->type == PHY_TYPE_PCIE) {
> >> +			/* gate_tx_pck_sel length select work for L1SS */
> >> +			writel(0xc0, priv->mmio + 0x74);
> >> +
> >> +			/* PLL KVCO tuning fine */
> >> +			rockchip_combphy_updatel(priv, GENMASK(4, 2), 0x4 << 2, 0x20 << 2);
> >> +
> >> +			/* Set up rx_trim: PLL LPF C1 85pf R1 1.25kohm */
> >> +			writel(0x4c, priv->mmio + (0x1b << 2));
> >> +
> >> +			/* Set up su_trim: T3_P1 650mv */
> >> +			writel(0x90, priv->mmio + (0xa << 2));
> >> +			writel(0x43, priv->mmio + (0xb << 2));
> >> +			writel(0x88, priv->mmio + (0xc << 2));
> >> +			writel(0x56, priv->mmio + (0xd << 2));
> >> +		} else if (priv->type == PHY_TYPE_SATA) {
> >> +			/* downward spread spectrum +500ppm */
> >> +			rockchip_combphy_updatel(priv, GENMASK(7, 4), 0x50, 0x1f << 2);
> >> +
> >> +			/* ssc ppm adjust to 3500ppm */
> >> +			rockchip_combphy_updatel(priv, GENMASK(3, 0), 0x7, 0x9 << 2);
> >> +		}
> >> +		break;
> >> +	default:
> >> +		dev_err(priv->dev, "Unsupported rate: %lu\n", rate);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	if (priv->ext_refclk) {
> >> +		rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_clk_ext, true);
> >> +		if (priv->type == PHY_TYPE_PCIE && rate == REF_CLOCK_100MHz) {
> >> +			writel(0x10, priv->mmio + (0x20 << 2));
> >> +
> >> +			writel(0x0c, priv->mmio + (0x1b << 2));
> >> +
> >> +			/* Set up su_trim: T3_P1 650mv */
> >> +			writel(0x90, priv->mmio + (0xa << 2));
> >> +			writel(0x43, priv->mmio + (0xb << 2));
> >> +			writel(0x88, 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);
> >> +
> >> +		if (priv->type == PHY_TYPE_PCIE && rate == REF_CLOCK_24MHz) {
> >> +			/* 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));
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> > 
> > 
> > 
> > 
> > _______________________________________________
> > Linux-rockchip mailing list
> > Linux-rockchip@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 
>