Index | Thread | Search

From:
Jonathan Matthew <jonathan@d14n.org>
Subject:
Re: dwmshc: add rk3528 support
To:
Mark Kettenis <mark.kettenis@xs4all.nl>
Cc:
tech@openbsd.org, aoglmf@gmail.com, kettenis@openbsd.org
Date:
Sun, 29 Mar 2026 11:51:26 +1000

Download raw body.

Thread
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 <jonathan@d14n.org>
> > 
> > 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