Index | Thread | Search

From:
Jonathan Matthew <jonathan@d14n.org>
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

Download raw body.

Thread
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
>