From: Jonathan Matthew Subject: Re: dwmshc: add rk3528 support To: tech@openbsd.org Cc: aoglmf@gmail.com, kettenis@openbsd.org Date: Sat, 28 Mar 2026 18:56:33 +1000 On Fri, Mar 27, 2026 at 04:30:50PM +1000, Jonathan Matthew wrote: > 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. I have now tested tnis on RK3568 as well. > > ok? > > 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 27 Mar 2026 05:44:38 -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_RK3528 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 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_RK3528, > + .rxclk = EMMC_DLL_RXCLK_RX_CLK_OUT_SEL, > + .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, > + .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, > + .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,16 @@ dwmshc_clock_post(struct sdhc_softc *sdh > if (dwmshc_dll_wait(sc) != 0) > return; > > + dll_val = 0; > + if (sc->sc_params->dll_tap_value != 0) { > + 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, EMMC_DLL_RXCLK_RX_CLK_OUT_SEL | > + dll_val); > + } > + 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 +429,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 >