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

23 Oct 2023

Hi Andre,
On 10/22/23 17:40, Andre Przywara wrote:
...
On Sat, 21 Oct 2023 22:52:06 -0500
Samuel Holland samuel@sholland.org wrote:
...
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.
Okay, that's fair. We can always revisit this later.
...
...
...

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.
The problem isn't in the writing, it's in the "~ptr" expression in the
if statement -- readl(chk) will always have zero in its high 32 bits,
while ~ptr will set the high 32 bits. So a cast after the inversion
would also fix the problem.
...
...
...


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.
Right, I'm fine with sending any fixes needed for D1 as follow-up. I'm
quite glad that we now have one version of the code that works on all
chip variants.
Regards,
Samuel

    