Re: [PATCH v2 16/22] sunxi: add R528/T113-s3/D1(s) DRAM initialisation code

[Andre Przywara <andre.przywara@arm.com>]

On Sat, 21 Oct 2023 22:52:06 -0500
Samuel Holland <samuel@sholland.org> wrote:

Hi Samuel,

> On 9/28/23 16:54, Andre Przywara wrote:
> > The Allwinner R528/T113-s/D1/D1s SoCs all share the same die, so use the
> > same DRAM initialisation code.
> > Make use of prior art here and lift some code from awboot[1], which
> > carried init code based on earlier decompilation efforts, but with a
> > GPL2 license tag.
> > This code has been heavily reworked and cleaned up, to match previous
> > DRAM routines for other SoCs, and also to be closer to U-Boot's coding
> > style and support routines.
> > The actual DRAM chip timing parameters are included in the main file,
> > since they cover all DRAM types, and are protected by a new Kconfig
> > CONFIG_SUNXI_DRAM_TYPE symbol, which allows the compiler to pick only
> > the relevant settings, at build time.
> > 
> > The relevant DRAM chips/board specific configuration parameters are
> > delivered via Kconfig, so this code here should work for all supported
> > SoCs and DRAM chips combinations.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > Tested-by: Sam Edwards <CFSworks@gmail.com>
> > ---
> >  drivers/Makefile                   |    1 +
> >  drivers/ram/Makefile               |    3 +
> >  drivers/ram/sunxi/Kconfig          |   59 ++
> >  drivers/ram/sunxi/Makefile         |    4 +
> >  drivers/ram/sunxi/dram_sun20i_d1.c | 1432 ++++++++++++++++++++++++++++
> >  drivers/ram/sunxi/dram_sun20i_d1.h |   73 ++
> >  6 files changed, 1572 insertions(+)
> >  create mode 100644 drivers/ram/sunxi/Makefile
> >  create mode 100644 drivers/ram/sunxi/dram_sun20i_d1.c
> >  create mode 100644 drivers/ram/sunxi/dram_sun20i_d1.h
> > 
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index efc2a4afb24..5a4bedf7305 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -56,6 +56,7 @@ obj-$(CONFIG_$(SPL_)ALTERA_SDRAM) += ddr/altera/
> >  obj-$(CONFIG_ARCH_IMX8M) += ddr/imx/imx8m/
> >  obj-$(CONFIG_IMX8ULP_DRAM) += ddr/imx/imx8ulp/
> >  obj-$(CONFIG_ARCH_IMX9) += ddr/imx/imx9/
> > +obj-$(CONFIG_DRAM_SUN8I_R528) += ram/  
> 
> This would need to be duplicated for DRAM_SUN20I_D1.
> 
> >  obj-$(CONFIG_SPL_DM_RESET) += reset/
> >  obj-$(CONFIG_SPL_MUSB_NEW) += usb/musb-new/
> >  obj-$(CONFIG_SPL_USB_GADGET) += usb/gadget/
> > diff --git a/drivers/ram/Makefile b/drivers/ram/Makefile
> > index 6eb1a241359..b4750ea11c4 100644
> > --- a/drivers/ram/Makefile
> > +++ b/drivers/ram/Makefile
> > @@ -23,6 +23,9 @@ obj-$(CONFIG_RAM_SIFIVE) += sifive/
> >  ifdef CONFIG_SPL_BUILD
> >  obj-$(CONFIG_SPL_STARFIVE_DDR) += starfive/
> >  endif
> > +
> > +obj-$(CONFIG_DRAM_SUN8I_R528) += sunxi/  
> 
> This would need to be duplicated for DRAM_SUN20I_D1.

Right, so I joined both symbols into one now. I think I wanted to keep
the DM_RAM and non-DM parts logically apart, but it works nevertheless.

My gut feeling is this needs more adjustments anyway once we need the
DM_RAM or want to extend it to more SoCs, so we can fix things when
they need fixing, later.

> 
> > +
> >  obj-$(CONFIG_ARCH_OCTEON) += octeon/
> >  
> >  obj-$(CONFIG_ARCH_RMOBILE) += renesas/
> > diff --git a/drivers/ram/sunxi/Kconfig b/drivers/ram/sunxi/Kconfig
> > index 261d7f57409..35eeda58efa 100644
> > --- a/drivers/ram/sunxi/Kconfig
> > +++ b/drivers/ram/sunxi/Kconfig
> > @@ -12,3 +12,62 @@ config DRAM_SUN8I_R528
> >  	default y if MACH_SUN8I_R528
> >  	help
> >  	  Select this DRAM controller driver for the R528/T113s SoCs.
> > +
> > +if DRAM_SUN20I_D1 || DRAM_SUN8I_R528
> > +
> > +config DRAM_SUNXI_ODT_EN  
> 
> You have a mixture of "DRAM_SUNXI" and "SUNXI_DRAM" for the tunables
> here. I would recommend being consistent.

This was chosen to stay consistent with the existing DRAM drivers,
which use DRAM_SUNxx_ for parameters, but SUNXI_DRAM_TYPE_ for the type
selection. I was looking into moving the other Allwinner DRAM drivers
into here as well (at least for every future SoC), and tested this with
the H616: keeping the symbols compatible was a requirement.

> 
> > +	hex "DRAM ODT EN parameter"
> > +	default 0x1
> > +	help
> > +	  ODT EN value from vendor DRAM settings.
> > +
> > +config DRAM_SUNXI_TPR0
> > +	hex "DRAM TPR0 parameter"
> > +	default 0x0
> > +	help
> > +	  TPR0 value from vendor DRAM settings.
> > +
> > +config DRAM_SUNXI_TPR11
> > +	hex "DRAM TPR11 parameter"
> > +	default 0x0
> > +	help
> > +	  TPR11 value from vendor DRAM settings.
> > +
> > +config DRAM_SUNXI_TPR12
> > +	hex "DRAM TPR12 parameter"
> > +	default 0x0
> > +	help
> > +	  TPR12 value from vendor DRAM settings.
> > +
> > +config DRAM_SUNXI_TPR13
> > +	hex "DRAM TPR13 parameter"
> > +	default 0x0  
> 
> I would suggest dropping the defaults for these tunables. It was
> non-obvious that I was missing some configuration when I switched to
> your driver. I think it's reasonable to require the defconfig/user to
> provide these.

Yeah, that's a good point. No actual value is 0, so this doesn't even
simplify defconfigs.

> > +	help
> > +	  TPR13 value from vendor DRAM settings. It tells which features
> > +	  should be configured.
> > +
> > +choice
> > +	prompt "DRAM chip type"
> > +	default SUNXI_DRAM_TYPE_DDR3 if DRAM_SUN8I_R528 || DRAM_SUN20I_D1
> > +
> > +config SUNXI_DRAM_TYPE_DDR2
> > +        bool "DDR2 chips"
> > +
> > +config SUNXI_DRAM_TYPE_DDR3
> > +        bool "DDR3 chips"
> > +
> > +config SUNXI_DRAM_TYPE_LPDDR2
> > +        bool "LPDDR2 chips"
> > +
> > +config SUNXI_DRAM_TYPE_LPDDR3
> > +        bool "LPDDR3 chips"
> > +endchoice
> > +
> > +config SUNXI_DRAM_TYPE
> > +	int
> > +	default 2 if SUNXI_DRAM_TYPE_DDR2
> > +	default 3 if SUNXI_DRAM_TYPE_DDR3
> > +	default 6 if SUNXI_DRAM_TYPE_LPDDR2
> > +	default 7 if SUNXI_DRAM_TYPE_LPDDR3
> > +
> > +endif
> > diff --git a/drivers/ram/sunxi/Makefile b/drivers/ram/sunxi/Makefile
> > new file mode 100644
> > index 00000000000..d6fb2cf0b65
> > --- /dev/null
> > +++ b/drivers/ram/sunxi/Makefile
> > @@ -0,0 +1,4 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +
> > +obj-$(CONFIG_DRAM_SUN20I_D1) += dram_sun20i_d1.o
> > +obj-$(CONFIG_DRAM_SUN8I_R528) += dram_sun20i_d1.o
> > diff --git a/drivers/ram/sunxi/dram_sun20i_d1.c b/drivers/ram/sunxi/dram_sun20i_d1.c
> > new file mode 100644
> > index 00000000000..c766fc24065
> > --- /dev/null
> > +++ b/drivers/ram/sunxi/dram_sun20i_d1.c
> > @@ -0,0 +1,1432 @@
> > [...]
> > +/*
> > + * This routine sizes a DRAM device by cycling through address lines and
> > + * figuring out if they are connected to a real address line, or if the
> > + * address is a mirror.
> > + * First the column and bank bit allocations are set to low values (2 and 9
> > + * address lines). Then a maximum allocation (16 lines) is set for rows and
> > + * this is tested.
> > + * Next the BA2 line is checked. This seems to be placed above the column,
> > + * BA0-1 and row addresses. Finally, the column address is allocated 13 lines
> > + * and these are tested. The results are placed in dram_para1 and dram_para2.
> > + */
> > +static int auto_scan_dram_size(const dram_para_t *para, dram_config_t *config)
> > +{
> > +	unsigned int rval, i, j, rank, maxrank, offs;
> > +	unsigned int shft;
> > +	unsigned long ptr, mc_work_mode, chk;  
> 
> This breaks on 64-bit architectures, because readl(chk) can never equal
> ~ptr. Please change ptr and chk to unsigned int.

I think they were originally ints (at least I see it like this is an
earlier version), but I changed it to make "ptr" a pointer compatible
type.
Definitely "chk" is only used as a pointer, so should be long (or
uintptr_t).
The actual problem is that "ptr" is used both as a pointer *and* a
payload, which is odd. I will keep ptr long, but make sure to
write only a true 32 bit payload into the address it points to.

> 
> > +
> > +	if (mctl_core_init(para, config) == 0) {
> > +		printf("DRAM initialisation error : 0\n");
> > +		return 0;
> > +	}
> > +
> > +	maxrank	= (config->dram_para2 & 0xf000) ? 2 : 1;
> > +	mc_work_mode = 0x3102000;
> > +	offs = 0;
> > +
> > +	/* write test pattern */
> > +	for (i = 0, ptr = CFG_SYS_SDRAM_BASE; i < 64; i++, ptr += 4)
> > +		writel((i & 0x1) ? ptr : ~ptr, ptr);
> > +
> > +	for (rank = 0; rank < maxrank;) {
> > +		/* set row mode */
> > +		clrsetbits_le32(mc_work_mode, 0xf0c, 0x6f0);
> > +		udelay(1);
> > +
> > +		// Scan per address line, until address wraps (i.e. see shadow)
> > +		for (i = 11; i < 17; i++) {
> > +			chk = CFG_SYS_SDRAM_BASE + (1U << (i + 11));
> > +			ptr = CFG_SYS_SDRAM_BASE;
> > +			for (j = 0; j < 64; j++) {
> > +				if (readl(chk) != ((j & 1) ? ptr : ~ptr))
> > +					break;
> > +				ptr += 4;
> > +				chk += 4;
> > +			}
> > +			if (j == 64)
> > +				break;
> > +		}
> > +		if (i > 16)
> > +			i = 16;
> > +		debug("rank %d row = %d\n", rank, i);
> > +
> > +		/* Store rows in para 1 */
> > +		shft = offs + 4;
> > +		rval = config->dram_para1;
> > +		rval &= ~(0xff << shft);
> > +		rval |= i << shft;
> > +		config->dram_para1 = rval;
> > +
> > +		if (rank == 1)		/* Set bank mode for rank0 */
> > +			clrsetbits_le32(0x3102000, 0xffc, 0x6a4);
> > +
> > +		/* Set bank mode for current rank */
> > +		clrsetbits_le32(mc_work_mode, 0xffc, 0x6a4);
> > +		udelay(1);
> > +
> > +		// Test if bit A23 is BA2 or mirror XXX A22?
> > +		chk = CFG_SYS_SDRAM_BASE + (1U << 22);
> > +		ptr = CFG_SYS_SDRAM_BASE;
> > +		for (i = 0, j = 0; i < 64; i++) {
> > +			if (readl(chk) != ((i & 1) ? ptr : ~ptr)) {
> > +				j = 1;
> > +				break;
> > +			}
> > +			ptr += 4;
> > +			chk += 4;
> > +		}
> > +
> > +		debug("rank %d bank = %d\n", rank, (j + 1) << 2); /* 4 or 8 */
> > +
> > +		/* Store banks in para 1 */
> > +		shft = 12 + offs;
> > +		rval = config->dram_para1;
> > +		rval &= ~(0xf << shft);
> > +		rval |= j << shft;
> > +		config->dram_para1 = rval;
> > +
> > +		if (rank == 1)		/* Set page mode for rank0 */
> > +			clrsetbits_le32(0x3102000, 0xffc, 0xaa0);
> > +
> > +		/* Set page mode for current rank */
> > +		clrsetbits_le32(mc_work_mode, 0xffc, 0xaa0);
> > +		udelay(1);
> > +
> > +		// Scan per address line, until address wraps (i.e. see shadow)
> > +		for (i = 9; i < 14; i++) {
> > +			chk = CFG_SYS_SDRAM_BASE + (1U << i);
> > +			ptr = CFG_SYS_SDRAM_BASE;
> > +			for (j = 0; j < 64; j++) {
> > +				if (readl(chk) != ((j & 1) ? ptr : ~ptr))
> > +					break;
> > +				ptr += 4;
> > +				chk += 4;
> > +			}
> > +			if (j == 64)
> > +				break;
> > +		}
> > +		if (i > 13)
> > +			i = 13;
> > +
> > +		unsigned int pgsize = (i == 9) ? 0 : (1 << (i - 10));
> > +		debug("rank %d page size = %d KB\n", rank, pgsize);
> > +
> > +		/* Store page size */
> > +		shft = offs;
> > +		rval = config->dram_para1;
> > +		rval &= ~(0xf << shft);
> > +		rval |= pgsize << shft;
> > +		config->dram_para1 = rval;
> > +
> > +		// Move to next rank
> > +		rank++;
> > +		if (rank != maxrank) {
> > +			if (rank == 1) {
> > +				/* MC_WORK_MODE */
> > +				clrsetbits_le32(0x3202000, 0xffc, 0x6f0);
> > +
> > +				/* MC_WORK_MODE2 */
> > +				clrsetbits_le32(0x3202004, 0xffc, 0x6f0);
> > +			}
> > +			/* store rank1 config in upper half of para1 */
> > +			offs += 16;
> > +			mc_work_mode += 4;	/* move to MC_WORK_MODE2 */
> > +		}
> > +	}
> > +	if (maxrank == 2) {
> > +		config->dram_para2 &= 0xfffff0ff;
> > +		/* note: rval is equal to para->dram_para1 here */
> > +		if ((rval & 0xffff) == (rval >> 16)) {
> > +			debug("rank1 config same as rank0\n");
> > +		} else {
> > +			config->dram_para2 |= BIT(8);
> > +			debug("rank1 config different from rank0\n");
> > +		}
> > +	}
> > +
> > +	return 1;
> > +}
> > [...]
> > +static int sunxi_ram_probe(struct udevice *dev)
> > +{
> > +	struct sunxi_ram_priv *priv = dev_get_priv(dev);
> > +	unsigned long dram_size;
> > +
> > +	debug("%s: %s: probing\n", __func__, dev->name);
> > +
> > +	dram_size = sunxi_dram_init();
> > +	if (!dram_size) {
> > +		printf("DRAM init failed: %d\n", ret);  
> 
> There is no ret variable anymore, so this fails to compile. With this
> line and the variable size issue fixed, this driver works on my Nezha
> board, so with those fixes:

Ah, right, thanks for the heads up. I think I had some trick to compile
test this code once, but didn't use it since last year.

> Tested-by: Samuel Holland <samuel@sholland.org>

Thanks!
Andre

P.S. This driver has a lot of rough edges anyway (the whole
clock/"system" setup side, for a start), but we should get things
moving now, before the RFC version celebrates its first birthday. So I
will merge it, but am happy to take fixes: either cleanups, or fixes
for the D1.

> > +		return -ENODEV;
> > +	}
> > +
> > +	priv->size = dram_size;
> > +
> > +	return 0;
> > +}  
>