On 20/05/2025 05:53, Yao Zi wrote:

On Mon, May 19, 2025 at 09:26:05PM +0200, Diederik de Haas wrote:

quoted

On Mon May 19, 2025 at 6:16 PM CEST, Yao Zi wrote:

quoted

All supported variants of naneng-combphy follow a register layout
similar to the RK3568 variant with some exceptions of SoC-specific
registers.

Add RK3568 prefix for the common set of registers and the corresponding
SoC prefix for SoC-specific registers, making usage of definitions clear
and preparing for future COMBPHY variants with a different register
layout.

Signed-off-by: Yao Zi <redacted>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
---
  .../rockchip/phy-rockchip-naneng-combphy.c    | 560 +++++++++---------
  1 file changed, 288 insertions(+), 272 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
index ce91fb1d5167..1d1c7723584b 100644
--- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c

@@ -21,78 +21,80 @@
  #define REF_CLOCK_100MHz		(100 * HZ_PER_MHZ)
  
  /* COMBO PHY REG */

<snip>
-#define PHYREG33_PLL_KVCO_VALUE_RK3576	4
+#define RK3568_PHYREG6				0x14
+#define RK3568_PHYREG6_PLL_DIV_MASK		GENMASK(7, 6)
+#define RK3568_PHYREG6_PLL_DIV_SHIFT		6
+#define RK3568_PHYREG6_PLL_DIV_2		1
+
+#define RK3568_PHYREG7				0x18
+#define RK3568_PHYREG7_TX_RTERM_MASK		GENMASK(7, 4)
+#define RK3568_PHYREG7_TX_RTERM_SHIFT		4
+#define RK3568_PHYREG7_TX_RTERM_50OHM		8
+#define RK3568_PHYREG7_RX_RTERM_MASK		GENMASK(3, 0)
+#define RK3568_PHYREG7_RX_RTERM_SHIFT		0
+#define RK3568_PHYREG7_RX_RTERM_44OHM		15
+
+#define RK3568_PHYREG8				0x1C
+#define RK3568_PHYREG8_SSC_EN			BIT(4)
+
+#define RK3568_PHYREG11				0x28
+#define RK3568_PHYREG11_SU_TRIM_0_7		0xF0
+
+#define RK3568_PHYREG12				0x2C
+#define RK3568_PHYREG12_PLL_LPF_ADJ_VALUE	4
+
+#define RK3568_PHYREG13				0x30
+#define RK3568_PHYREG13_RESISTER_MASK		GENMASK(5, 4)
+#define RK3568_PHYREG13_RESISTER_SHIFT		0x4
+#define RK3568_PHYREG13_RESISTER_HIGH_Z		3
+#define RK3568_PHYREG13_CKRCV_AMP0		BIT(7)
+
+#define RK3568_PHYREG14				0x34
+#define RK3568_PHYREG14_CKRCV_AMP1		BIT(0)
+
+#define RK3568_PHYREG15				0x38
+#define RK3568_PHYREG15_CTLE_EN			BIT(0)
+#define RK3568_PHYREG15_SSC_CNT_MASK		GENMASK(7, 6)
+#define RK3568_PHYREG15_SSC_CNT_SHIFT		6
+#define RK3568_PHYREG15_SSC_CNT_VALUE		1
+
+#define RK3568_PHYREG16				0x3C
+#define RK3568_PHYREG16_SSC_CNT_VALUE		0x5f
+
+#define RK3568_PHYREG18				0x44
+#define RK3568_PHYREG18_PLL_LOOP		0x32
+
+#define RK3568_PHYREG32				0x7C
+#define RK3568_PHYREG32_SSC_MASK		GENMASK(7, 4)
+#define RK3568_PHYREG32_SSC_DIR_MASK		GENMASK(5, 4)
+#define RK3568_PHYREG32_SSC_DIR_SHIFT		4
+#define RK3568_PHYREG32_SSC_UPWARD		0
+#define RK3568_PHYREG32_SSC_DOWNWARD		1
+#define RK3568_PHYREG32_SSC_OFFSET_MASK	GENMASK(7, 6)
+#define RK3568_PHYREG32_SSC_OFFSET_SHIFT	6
+#define RK3568_PHYREG32_SSC_OFFSET_500PPM	1
+
+#define RK3568_PHYREG33				0x80
+#define RK3568_PHYREG33_PLL_KVCO_MASK		GENMASK(4, 2)
+#define RK3568_PHYREG33_PLL_KVCO_SHIFT		2
+#define RK3568_PHYREG33_PLL_KVCO_VALUE		2
+#define RK3576_PHYREG33_PLL_KVCO_VALUE		4
+
+/* RK3588 COMBO PHY registers */
+#define RK3588_PHYREG27				0x6C
+#define RK3588_PHYREG27_RX_TRIM			0x4C

Would it be better if RK3588_PHYREG* comes after RK3576_PHYREG*?

Cheers,
   Diederik

It's intended to keep RK3576 definitions below RK3588 ones. The RK3576
driver makes use of a register introduced for RK3588 variant
(RK3588_PHYREG27). Since similar reusing doesn't happen reversely, I
consider the design of RK3576 a superset of the RK3588 one, and put
RK3576 definitions later in the file.

Sound logic, RK3576 was announced after RK3588, thus the order makes sense.

Add my:
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

quoted

quoted

+
+/* RK3576 COMBO PHY registers */
+#define RK3576_PHYREG10				0x24
+#define RK3576_PHYREG10_SSC_PCM_MASK		GENMASK(3, 0)
+#define RK3576_PHYREG10_SSC_PCM_3500PPM		7
+
+#define RK3576_PHYREG17				0x40
+
+#define RK3576_PHYREG21				0x50
+#define RK3576_PHYREG21_RX_SQUELCH_VAL		0x0D
+
+#define RK3576_PHYREG30				0x74
  
  struct rockchip_combphy_priv;
<snip>

Thanks,
Yao Zi