From: Mark Kettenis Subject: Re: rkcomphy: add rk3528 support To: Jonathan Matthew Cc: tech@openbsd.org, kettenis@openbsd.org Date: Tue, 24 Mar 2026 21:52:35 +0100 > Date: Mon, 23 Mar 2026 15:22:46 +1000 > From: Jonathan Matthew Hey, > > On Sun, Mar 22, 2026 at 07:17:48PM +0100, Mark Kettenis wrote: > > > Date: Sun, 22 Mar 2026 21:48:59 +1000 > > > From: Jonathan Matthew > > > > > > This adds support for the RK3528 variant of the combo phy, > > > currently only in PCIe mode. It looks like it can do USB3 too > > > but I don't see any devices that do that. Hopefully a TRM for > > > the 3528 will turn up at some point so we can do this properly. > > > > > > The rkclock change just ignores attempts at setting the pcie inner > > > phy clock rate, since it's fixed at 100Mhz but the device tree > > > indicates the driver should set it. > > > > > > With this I can get pcie and the second nic on a Radxa E20C working > > > with u-boot 2026.04rc4 and some device tree bits pulled from linux. > > > > > > ok? > > > > Can we implement that clock properly? > > I was hoping to get away with just copying what u-boot does.. > I think I've deciphered the linux macro maze correctly; the updated diff > still works, at least. > > > > + > > > + reg = HREAD4(sc, COMBO_PIPE_PHY_REG(31)); > > > + reg &= ~COMBO_PIPE_PHY_SSC_OFFSET_MASK; > > > + reg &= ~COMBO_PIPE_PHY_SSC_DIR_MASK; > > > + reg |= COMBO_PIPE_PHY_SSC_DIR_DOWN; > > > + HWRITE4(sc, COMBO_PIPE_PHY_REG(31), reg); > > > > I don't see writes to this register in the Linux driver. Instead it > > writes to RK3528_PHYREG6 (which would be register 5 in our driver). > > Right, I forgot to update the register number. It appears the linux driver > uses 0 based numbering for the 3528 registers, and the numbers are also > base 16, unlike the other chips: > > #define RK3528_PHYREG6 0x18 > .. > #define RK3528_PHYREG40 0x100 > > so I've changed that to COMBO_PIPE_PHY_REG(6), and adjusted the > other register numbers. > > Index: rkclock_clocks.h > =================================================================== > RCS file: /cvs/src/sys/dev/fdt/rkclock_clocks.h,v > diff -u -p -r1.68 rkclock_clocks.h > --- rkclock_clocks.h 12 Mar 2026 20:44:38 -0000 1.68 > +++ rkclock_clocks.h 23 Mar 2026 05:19:35 -0000 > @@ -297,6 +297,8 @@ > #define RK3528_CLK_MATRIX_200M_SRC 10 > #define RK3528_CLK_PWM0 111 > #define RK3528_CLK_PWM1 114 > +#define RK3528_CLK_PPLL_100M_MATRIX 121 > +#define RK3528_CLK_REF_PCIE_INNER_PHY 123 > #define RK3528_CLK_PPLL_125M_MATRIX 127 > #define RK3528_CCLK_SRC_EMMC 140 > #define RK3528_BCLK_EMMC 143 > Index: rkclock.c > =================================================================== > RCS file: /cvs/src/sys/dev/fdt/rkclock.c,v > diff -u -p -r1.96 rkclock.c > --- rkclock.c 12 Mar 2026 20:44:38 -0000 1.96 > +++ rkclock.c 23 Mar 2026 05:19:35 -0000 > @@ -176,6 +176,7 @@ > #define RK3528_CRU_GATE_CON(i) (0x00800 + (i) * 4) > #define RK3528_CRU_SOFTRST_CON(i) (0x00a00 + (i) * 4) > #define RK3528_PCIE_CRU_PLL_CON(i) (0x20000 + (i) * 4) > +#define RK3528_PCIE_CLKSEL_CON(i) (0x20300 + (i) * 4) > > /* RK3568 registers */ > #define RK3568_CRU_APLL_CON(i) (0x0000 + (i) * 4) > @@ -3219,6 +3220,16 @@ const struct rkclock rk3528_clocks[] = { > SEL(9, 8), 0, > { RK3528_CLK_MATRIX_100M_SRC, RK3528_CLK_MATRIX_50M_SRC, > RK3528_XIN24M } > + }, > + { > + RK3528_CLK_PPLL_100M_MATRIX, RK3528_PCIE_CLKSEL_CON(1), > + 0, DIV(7, 2), > + { RK3528_PLL_PPLL } > + }, Hayk already mentioned the mistake in DIV(). > + { > + RK3528_CLK_REF_PCIE_INNER_PHY, RK3528_PCIE_CLKSEL_CON(1), > + SEL(13, 13), 0, > + { RK3528_CLK_PPLL_100M_MATRIX, RK3528_XIN24M } > }, > { > RK3528_CLK_PPLL_125M_MATRIX, RK3528_CRU_CLKSEL_CON(60), > Index: rkcomphy.c > =================================================================== > RCS file: /cvs/src/sys/dev/fdt/rkcomphy.c,v > diff -u -p -r1.2 rkcomphy.c > --- rkcomphy.c 27 Apr 2023 08:56:39 -0000 1.2 > +++ rkcomphy.c 23 Mar 2026 05:19:35 -0000 > @@ -36,9 +36,13 @@ > > /* Combo PHY registers */ > #define COMBO_PIPE_PHY_REG(idx) ((idx) * 4) > +/* REG_004 */ > +#define COMBO_PIPE_PHY_GATE_TX_PCK_DLY_PLL_OFF (1 << 3) > /* REG_005 */ > #define COMBO_PIPE_PHY_PLL_DIV_MASK (0x3 << 6) > #define COMBO_PIPE_PHY_PLL_DIV_2 (0x1 << 6) > +#define COMBO_PIPE_PHY_PLL_KVCO_MASK_RK3528 (0x7 << 10) > +#define COMBO_PIPE_PHY_PLL_KVCO_VALUE_RK3528 (0x2 << 10) > /* REG_006 */ > #define COMBO_PIPE_PHY_TX_RTERM_50OHM (0x8 << 4) > #define COMBO_PIPE_PHY_RX_RTERM_44OHM (0xf << 4) > @@ -124,6 +128,7 @@ struct cfdriver rkcomphy_cd = { > NULL, "rkcomphy", DV_DULL > }; > > +int rkcomphy_rk3528_enable(void *, uint32_t *); > int rkcomphy_rk3568_enable(void *, uint32_t *); > int rkcomphy_rk3588_enable(void *, uint32_t *); > > @@ -133,7 +138,8 @@ rkcomphy_match(struct device *parent, vo > struct fdt_attach_args *faa = aux; > int node = faa->fa_node; > > - return OF_is_compatible(node, "rockchip,rk3568-naneng-combphy") || > + return OF_is_compatible(node, "rockchip,rk3528-naneng-combphy") || > + OF_is_compatible(node, "rockchip,rk3568-naneng-combphy") || > OF_is_compatible(node, "rockchip,rk3588-naneng-combphy"); > } > > @@ -161,11 +167,87 @@ rkcomphy_attach(struct device *parent, s > > sc->sc_pd.pd_node = faa->fa_node; > sc->sc_pd.pd_cookie = sc; > - if (OF_is_compatible(faa->fa_node, "rockchip,rk3568-naneng-combphy")) > + if (OF_is_compatible(faa->fa_node, "rockchip,rk3528-naneng-combphy")) > + sc->sc_pd.pd_enable = rkcomphy_rk3528_enable; > + else if (OF_is_compatible(faa->fa_node, > + "rockchip,rk3568-naneng-combphy")) > sc->sc_pd.pd_enable = rkcomphy_rk3568_enable; > else > sc->sc_pd.pd_enable = rkcomphy_rk3588_enable; > phy_register(&sc->sc_pd); > +} > + > +void > +rkcomphy_rk3528_pll_tune(struct rkcomphy_softc *sc) > +{ > + uint32_t reg; > + > + reg = HREAD4(sc, COMBO_PIPE_PHY_REG(5)); > + reg |= COMBO_PIPE_PHY_GATE_TX_PCK_DLY_PLL_OFF; > + HWRITE4(sc, COMBO_PIPE_PHY_REG(5), reg); > + > + reg = HREAD4(sc, COMBO_PIPE_PHY_REG(6)); > + reg &= ~COMBO_PIPE_PHY_PLL_KVCO_MASK_RK3528; > + reg |= COMBO_PIPE_PHY_PLL_KVCO_VALUE_RK3528; > + HWRITE4(sc, COMBO_PIPE_PHY_REG(6), reg); > + > + /* su_trim[6:4]=111, [10:7]=1001, [2:0]=000, swing 650mv */ > + HWRITE4(sc, COMBO_PIPE_PHY_REG(0x42), 0x570804f0); Fun, so now the Linux driver has rgeister names that are off-by-one, register names that are not of-by-one and register names that are hexadecimal. All in one driver. It is a bit of a tossup I guess, but I'd probably consistently use decimal. So COMBO_PIPE_PHY_REG(66). Either way, ok kettenis@ (with the DIV thing fixed) > +} > + > +int > +rkcomphy_rk3528_enable(void *cookie, uint32_t *cells) > +{ > + struct rkcomphy_softc *sc = cookie; > + struct regmap *rm, *phy_rm; > + int node = sc->sc_pd.pd_node; > + uint32_t type = cells[0]; > + uint32_t grf, phy_grf, reg; > + > + /* We only support PCIe for now. (maybe USB3 later?) */ > + switch (type) { > + case PHY_TYPE_PCIE: > + break; > + default: > + return EINVAL; > + } > + > + grf = OF_getpropint(node, "rockchip,pipe-grf", 0); > + rm = regmap_byphandle(grf); > + if (rm == NULL) > + return ENXIO; > + > + phy_grf = OF_getpropint(node, "rockchip,pipe-phy-grf", 0); > + phy_rm = regmap_byphandle(phy_grf); > + if (phy_rm == NULL) > + return ENXIO; > + > + clock_set_assigned(node); > + clock_enable_all(node); > + > + reg = HREAD4(sc, COMBO_PIPE_PHY_REG(6)); > + reg &= ~COMBO_PIPE_PHY_SSC_OFFSET_MASK; > + reg &= ~COMBO_PIPE_PHY_SSC_DIR_MASK; > + reg |= COMBO_PIPE_PHY_SSC_DIR_DOWN; > + HWRITE4(sc, COMBO_PIPE_PHY_REG(6), reg); > + > + switch (type) { > + case PHY_TYPE_PCIE: > + regmap_write_4(phy_rm, PIPE_PHY_GRF_PIPE_CON(0), 0xffff0110); > + regmap_write_4(phy_rm, PIPE_PHY_GRF_PIPE_CON(1), 0xffff0000); > + regmap_write_4(phy_rm, PIPE_PHY_GRF_PIPE_CON(2), 0xffff0101); > + regmap_write_4(phy_rm, PIPE_PHY_GRF_PIPE_CON(3), 0xffff0200); > + break; > + } > + > + regmap_write_4(phy_rm, PIPE_PHY_GRF_PIPE_CON(1), > + PIPE_PHY_GRF_PIPE_CLK_100M); > + if (type == PHY_TYPE_PCIE) > + rkcomphy_rk3528_pll_tune(sc); > + > + reset_deassert_all(node); > + > + return 0; > } > > void >