Message-ID: <1904911.CQOukoFCf9@diego>
Date: Tue, 15 Oct 2024 10:47:23 +0200
From: Heiko Stübner <heiko@...ech.de>
To: vkoul@...nel.org, kishon@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, Frank Wang <frawang.cn@...il.com>,
 Shawn Lin <shawn.lin@...k-chips.com>
Cc: shawn.lin@...k-chips.com, linux-phy@...ts.infradead.org,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org,
 william.wu@...k-chips.com, tim.chen@...k-chips.com,
 Kever Yang <kever.yang@...k-chips.com>,
 Frank Wang <frank.wang@...k-chips.com>
Subject: Re: [PATCH 2/2] phy: rockchip-naneng-combo: Support rk3576

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@...k-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@...ts.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 
> 

Powered by blists - more mailing lists