From: Jonathan Matthew Subject: Re: dwmshc: add rk3528 support To: Mark Kettenis Cc: tech@openbsd.org, aoglmf@gmail.com, kettenis@openbsd.org Date: Sun, 29 Mar 2026 11:51:26 +1000 On Sat, Mar 28, 2026 at 01:26:19PM +0100, Mark Kettenis wrote: > > Date: Fri, 27 Mar 2026 16:30:50 +1000 > > From: Jonathan Matthew > > > > dwmshc(4) attaches to the RK3528 emmc controller, because the device tree > > says it's compatible with the 3588 variant, and sd(4) attaches successsfully > > and looks like it'll work, but i/o operations to the device fail. > > > > To make it actually work, it needs a few different settings to the 3568 > > and 3588. I decided that just adding more if-else blocks was making it > > a bit hard to follow, so I extracted the different register values to a > > per-model structure. > > > > For the RK3528 controller, we have to manually set the tap values to twice > > the DLL lock value rather than letting the controller do it. It doesn't > > work otherwise. This means the write to the RXCLK register has to happen > > after the call to dwmshc_dll_wait() in order to get a valid read of the > > DLL lock value. > > > > I've tested this on RK3588 without problems, but not RK356x yet. > > > > ok? > > A few nits below. Otherwise this looks ok. All good points. This improved version still works: Index: dwmshc.c =================================================================== RCS file: /cvs/src/sys/dev/fdt/dwmshc.c,v diff -u -p -r1.8 dwmshc.c --- dwmshc.c 15 Jul 2024 09:56:30 -0000 1.8 +++ dwmshc.c 29 Mar 2026 01:49:23 -0000 @@ -102,6 +102,7 @@ #define EMMC_DLL_TXCLK_TX_TAP_NUM_SHIFT 0 #define EMMC_DLL_TXCLK_TX_TAP_NUM_MASK 0x1f #define EMMC_DLL_TXCLK_TX_TAP_NUM_90_DEG 0x8 +#define EMMC_DLL_TXCLK_TX_TAP_NUM_135_DEG 0xc #define EMMC_DLL_TXCLK_TX_TAP_NUM_DEFAULT 0x10 #define EMMC_DLL_TXCLK_TX_TAP_VALUE_SHIFT 8 #define EMMC_DLL_TXCLK_TX_TAP_VALUE_MASK 0xff @@ -157,10 +158,18 @@ #define EMMC_DLL_STATUS1_DLL_STRBIN_DELAY_VALUE_MASK \ 0xff +struct dwmshc_params { + const char *compatible; + uint32_t txclk_tapnum; + uint32_t rxclk; + int use_dll_tap_value; +}; + struct dwmshc_softc { struct sdhc_softc sc_sdhc; int sc_node; + const struct dwmshc_params *sc_params; bus_space_tag_t sc_iot; bus_space_handle_t sc_ioh; @@ -193,14 +202,43 @@ struct cfdriver dwmshc_cd = { NULL, "dwmshc", DV_DULL }; +static const struct dwmshc_params dwmshc_hw_params[] = { + { + .compatible = "rockchip,rk3528-dwcmshc", + .txclk_tapnum = EMMC_DLL_TXCLK_TX_TAP_NUM_135_DEG, + .rxclk = EMMC_DLL_RXCLK_RX_CLK_OUT_SEL, + .use_dll_tap_value = 1 + }, + { + .compatible = "rockchip,rk3568-dwcmshc", + .txclk_tapnum = EMMC_DLL_TXCLK_TX_TAP_NUM_DEFAULT, + .rxclk = EMMC_DLL_RXCLK_RX_CLK_OUT_SEL | + EMMC_DLL_RXCLK_RX_CLK_SRC_SEL, + .use_dll_tap_value = 0 + }, + { + .compatible = "rockchip,rk3588-dwcmshc", + .txclk_tapnum = EMMC_DLL_TXCLK_TX_TAP_NUM_DEFAULT, + .rxclk = EMMC_DLL_RXCLK_RX_CLK_OUT_SEL, + .use_dll_tap_value = 0 + } +}; + int dwmshc_match(struct device *parent, void *match, void *aux) { struct fdt_attach_args *faa = aux; + int i; + + for (i = 0; i < nitems(dwmshc_hw_params); i++) { + if (OF_is_compatible(faa->fa_node, + dwmshc_hw_params[i].compatible)) + return 1; + } - return (OF_is_compatible(faa->fa_node, "rockchip,rk3568-dwcmshc") || - OF_is_compatible(faa->fa_node, "rockchip,rk3588-dwcmshc")); + return 0; } + static void dwmshc_attach(struct device *parent, struct device *self, void *aux) { @@ -209,7 +247,7 @@ dwmshc_attach(struct device *parent, str struct fdt_attach_args *faa = aux; uint64_t capmask = 0; uint16_t capset = 0; - int bus_width; + int bus_width, i; uint32_t freq; sc->sc_node = faa->fa_node; @@ -228,6 +266,14 @@ dwmshc_attach(struct device *parent, str return; } + for (i = 0; i < nitems(dwmshc_hw_params); i++) { + if (OF_is_compatible(sc->sc_node, + dwmshc_hw_params[i].compatible)) { + sc->sc_params = &dwmshc_hw_params[i]; + break; + } + } + pinctrl_byname(sc->sc_node, "default"); clock_set_assigned(sc->sc_node); @@ -320,7 +366,8 @@ static void dwmshc_clock_post(struct sdhc_softc *sdhc, int freq, int timing) { struct dwmshc_softc *sc = (struct dwmshc_softc *)sdhc; - uint32_t txclk_tapnum = EMMC_DLL_TXCLK_TX_TAP_NUM_DEFAULT; + uint32_t txclk_tapnum = sc->sc_params->txclk_tapnum; + uint32_t dll_val; clock_set_frequency(sc->sc_node, 0, freq * 1000); @@ -341,11 +388,6 @@ dwmshc_clock_post(struct sdhc_softc *sdh delay(1); dwmshc_wr4(sc, EMMC_DLL_CTRL, 0); - if (OF_is_compatible(sc->sc_node, "rockchip,rk3568-dwcmshc")) - dwmshc_wr4(sc, EMMC_DLL_RXCLK, EMMC_DLL_RXCLK_RX_CLK_OUT_SEL | - EMMC_DLL_RXCLK_RX_CLK_SRC_SEL); - else - dwmshc_wr4(sc, EMMC_DLL_RXCLK, EMMC_DLL_RXCLK_RX_CLK_OUT_SEL); dwmshc_wr4(sc, EMMC_DLL_CTRL, EMMC_DLL_CTRL_DLL_START | 0x5 << EMMC_DLL_CTRL_DLL_START_POINT_SHIFT | 0x2 << EMMC_DLL_CTRL_DLL_INCREMENT_SHIFT); @@ -353,6 +395,14 @@ dwmshc_clock_post(struct sdhc_softc *sdh if (dwmshc_dll_wait(sc) != 0) return; + dll_val = 0; + if (sc->sc_params->use_dll_tap_value) { + dll_val = EMMC_DLL_RXCLK_RX_TAP_VALUE_SEL | + ((dwmshc_rd4(sc, EMMC_DLL_STATUS0) & + EMMC_DLL_STATUS0_DLL_LOCK_VALUE_MASK) * 2); + } + dwmshc_wr4(sc, EMMC_DLL_RXCLK, sc->sc_params->rxclk | dll_val); + dwmshc_wr4(sc, EMMC_AT_CTRL, EMMC_AT_CTRL_TUNE_CLK_STOP_EN | EMMC_AT_CTRL_PRE_CHANGE_DLY_LT4 | EMMC_AT_CTRL_POST_CHANGE_DLY_LT4); @@ -377,11 +427,12 @@ dwmshc_clock_post(struct sdhc_softc *sdh dwmshc_wr4(sc, EMMC_DLL_TXCLK, EMMC_DLL_TXCLK_TX_CLK_OUT_SEL | EMMC_DLL_TXCLK_TX_TAP_NUM_SEL | - txclk_tapnum << EMMC_DLL_TXCLK_TX_TAP_NUM_SHIFT); + txclk_tapnum << EMMC_DLL_TXCLK_TX_TAP_NUM_SHIFT | + dll_val); dwmshc_wr4(sc, EMMC_DLL_STRBIN, EMMC_DLL_STRBIN_DELAY_ENA | EMMC_DLL_STRBIN_TAP_NUM_SEL | (EMMC_DLL_STRBIN_TAP_NUM_90_DEG << - EMMC_DLL_STRBIN_TAP_NUM_SHIFT)); + EMMC_DLL_STRBIN_TAP_NUM_SHIFT) | dll_val); } static int