On Sat, 21 Oct 2023 22:52:06 -0500
Samuel Holland <sam...@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.przyw...@arm.com>
> > Tested-by: Sam Edwards <cfswo...@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 <sam...@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;
> > +}
>
