Index | Thread | Search

From:
Mark Kettenis <mark.kettenis@xs4all.nl>
Subject:
Re: dwmshc: add rk3528 support
To:
Jonathan Matthew <jonathan@d14n.org>
Cc:
tech@openbsd.org, aoglmf@gmail.com, kettenis@openbsd.org
Date:
Sat, 28 Mar 2026 13:28:58 +0100

Download raw body.

Thread
> Date: Sat, 28 Mar 2026 18:56:33 +1000
> From: Jonathan Matthew <jonathan@d14n.org>
> 
> 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.

Good 'cause I seem to have misplaced mine.

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