[PATCH v2 16/22] sunxi: add R528/T113-s3/D1(s) DRAM initialisation code
Samuel Holland
samuel at sholland.org
Mon Oct 23 04:58:29 CEST 2023
Hi Andre,
On 10/22/23 17:40, Andre Przywara wrote:
> On Sat, 21 Oct 2023 22:52:06 -0500
> Samuel Holland <samuel at 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 at arm.com>
>>> Tested-by: Sam Edwards <CFSworks at 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 at 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
More information about the U-Boot
mailing list