On Fri, 11 Apr 2025 18:14:39 +0200
Jernej Skrabec <jernej.skra...@gmail.com> wrote:
Hi Jernej,
> H616 rank and size detection code is superior to the H6. Nevertheless,
> they are structurally the same. Split functions from H616 into new file
> and reuse them in H6 DRAM driver too. This should also fix some bugs for
> H6 too, like incorrect DRAM size detection.
that's a nice patch, but actually breaks on my Pine H64:
U-Boot SPL 2025.04-01089-g14694431269f (Apr 15 2025 - 22:55:07 +0100)
DRAM:DRAM init failed: Can't detect number of columns!
resetting ...
It's none of the previous H6 reworks that causes the problem, but it's
actually already in the current code somehow: If I apply the equivalent
of [PATCH 1/5] to the H6 code, it already complains. Replacing the
panic with a printf() gives me (on mainline):
U-Boot SPL 2025.04-01085-g7a43fe97dc86-dirty (Apr 15 2025 - 23:48:42 +0100)
DRAM:detected 14 rows (found=1)
detected 11 columns (found=0)
2048 MiB
Trying to boot from FEL
NOTICE: BL31: v2.12.0(debug):v2.12.0-724-gf745e004a
< continues booting >
So it seems like the match failed, but apparently 11 columns are correct anyway.
And since the same happens after this patch, I assume that even the
newer and better matching routine doesn't change that, unfortunately.
The config on this board is: 2 ranks, full width, 11 cols, 14 rows => 2GB.
And that seems to work (in Linux).
It's a single chip Elpida FA232A2MA JD-F, listed by Micron as
EDFA232A2MA-JD-F-D, though I couldn't find a datasheet quickly.
Cheers,
Andre
> Signed-off-by: Jernej Skrabec <jernej.skra...@gmail.com>
> ---
> .../include/asm/arch-sunxi/dram_dw_helpers.h | 22 +++
> arch/arm/mach-sunxi/Makefile | 4 +-
> arch/arm/mach-sunxi/dram_dw_helpers.c | 160 ++++++++++++++++++
> arch/arm/mach-sunxi/dram_sun50i_h6.c | 91 +---------
> arch/arm/mach-sunxi/dram_sun50i_h616.c | 155 +----------------
> 5 files changed, 190 insertions(+), 242 deletions(-)
> create mode 100644 arch/arm/include/asm/arch-sunxi/dram_dw_helpers.h
> create mode 100644 arch/arm/mach-sunxi/dram_dw_helpers.c
>
> diff --git a/arch/arm/include/asm/arch-sunxi/dram_dw_helpers.h
> b/arch/arm/include/asm/arch-sunxi/dram_dw_helpers.h
> new file mode 100644
> index 000000000000..bc9e0d868c55
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-sunxi/dram_dw_helpers.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Helpers that are commonly used with DW memory controller.
> + *
> + * (C) Copyright 2025 Jernej Skrabec <jernej.skra...@gmail.com>
> + *
> + */
> +
> +#ifndef _DRAM_DW_HELPERS_H
> +#define _DRAM_DW_HELPERS_H
> +
> +#include <asm/arch/dram.h>
> +
> +bool mctl_core_init(const struct dram_para *para,
> + const struct dram_config *config);
> +void mctl_auto_detect_rank_width(const struct dram_para *para,
> + struct dram_config *config);
> +void mctl_auto_detect_dram_size(const struct dram_para *para,
> + struct dram_config *config);
> +unsigned long mctl_calc_size(const struct dram_config *config);
> +
> +#endif
> diff --git a/arch/arm/mach-sunxi/Makefile b/arch/arm/mach-sunxi/Makefile
> index eb6a49119a13..a33cd5b0f07a 100644
> --- a/arch/arm/mach-sunxi/Makefile
> +++ b/arch/arm/mach-sunxi/Makefile
> @@ -41,8 +41,8 @@ obj-$(CONFIG_DRAM_SUN9I) += dram_sun9i.o
> obj-$(CONFIG_SPL_SPI_SUNXI) += spl_spi_sunxi.o
> obj-$(CONFIG_SUNXI_DRAM_DW) += dram_sunxi_dw.o
> obj-$(CONFIG_SUNXI_DRAM_DW) += dram_timings/
> -obj-$(CONFIG_DRAM_SUN50I_H6) += dram_sun50i_h6.o
> +obj-$(CONFIG_DRAM_SUN50I_H6) += dram_sun50i_h6.o dram_dw_helpers.o
> obj-$(CONFIG_DRAM_SUN50I_H6) += dram_timings/
> -obj-$(CONFIG_DRAM_SUN50I_H616) += dram_sun50i_h616.o
> +obj-$(CONFIG_DRAM_SUN50I_H616) += dram_sun50i_h616.o dram_dw_helpers.o
> obj-$(CONFIG_DRAM_SUN50I_H616) += dram_timings/
> endif
> diff --git a/arch/arm/mach-sunxi/dram_dw_helpers.c
> b/arch/arm/mach-sunxi/dram_dw_helpers.c
> new file mode 100644
> index 000000000000..885d1f2c0b12
> --- /dev/null
> +++ b/arch/arm/mach-sunxi/dram_dw_helpers.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Helpers that are commonly used with DW memory controller.
> + *
> + * (C) Copyright 2025 Jernej Skrabec <jernej.skra...@gmail.com>
> + *
> + */
> +
> +#include <init.h>
> +#include <asm/arch/dram_dw_helpers.h>
> +
> +void mctl_auto_detect_rank_width(const struct dram_para *para,
> + struct dram_config *config)
> +{
> + /* this is minimum size that it's supported */
> + config->cols = 8;
> + config->rows = 13;
> +
> + /*
> + * Strategy here is to test most demanding combination first and least
> + * demanding last, otherwise HW might not be fully utilized. For
> + * example, half bus width and rank = 1 combination would also work
> + * on HW with full bus width and rank = 2, but only 1/4 RAM would be
> + * visible.
> + */
> +
> + debug("testing 32-bit width, rank = 2\n");
> + config->bus_full_width = 1;
> + config->ranks = 2;
> + if (mctl_core_init(para, config))
> + return;
> +
> + debug("testing 32-bit width, rank = 1\n");
> + config->bus_full_width = 1;
> + config->ranks = 1;
> + if (mctl_core_init(para, config))
> + return;
> +
> + debug("testing 16-bit width, rank = 2\n");
> + config->bus_full_width = 0;
> + config->ranks = 2;
> + if (mctl_core_init(para, config))
> + return;
> +
> + debug("testing 16-bit width, rank = 1\n");
> + config->bus_full_width = 0;
> + config->ranks = 1;
> + if (mctl_core_init(para, config))
> + return;
> +
> + panic("This DRAM setup is currently not supported.\n");
> +}
> +
> +static void mctl_write_pattern(void)
> +{
> + unsigned int i;
> + u32 *ptr, val;
> +
> + ptr = (u32 *)CFG_SYS_SDRAM_BASE;
> + for (i = 0; i < 16; ptr++, i++) {
> + if (i & 1)
> + val = ~(ulong)ptr;
> + else
> + val = (ulong)ptr;
> + writel(val, ptr);
> + }
> +}
> +
> +static bool mctl_check_pattern(ulong offset)
> +{
> + unsigned int i;
> + u32 *ptr, val;
> +
> + ptr = (u32 *)CFG_SYS_SDRAM_BASE;
> + for (i = 0; i < 16; ptr++, i++) {
> + if (i & 1)
> + val = ~(ulong)ptr;
> + else
> + val = (ulong)ptr;
> + if (val != *(ptr + offset / 4))
> + return false;
> + }
> +
> + return true;
> +}
> +
> +void mctl_auto_detect_dram_size(const struct dram_para *para,
> + struct dram_config *config)
> +{
> + unsigned int shift, cols, rows, found;
> + u32 buffer[16];
> +
> + /* max. config for columns, but not rows */
> + config->cols = 11;
> + config->rows = 13;
> + mctl_core_init(para, config);
> +
> + /*
> + * Store content so it can be restored later. This is important
> + * if controller was already initialized and holds any data
> + * which is important for restoring system.
> + */
> + memcpy(buffer, (u32 *)CFG_SYS_SDRAM_BASE, sizeof(buffer));
> +
> + mctl_write_pattern();
> +
> + shift = config->bus_full_width + 1;
> +
> + /* detect column address bits */
> + found = 0;
> + for (cols = 8; cols < 11; cols++) {
> + if (mctl_check_pattern(1ULL << (cols + shift))) {
> + found = 1;
> + break;
> + }
> + }
> + if (!found)
> + panic("DRAM init failed: Can't detect number of columns!");
> + debug("detected %u columns\n", cols);
> +
> + /* restore data */
> + memcpy((u32 *)CFG_SYS_SDRAM_BASE, buffer, sizeof(buffer));
> +
> + /* reconfigure to make sure that all active rows are accessible */
> + config->cols = 8;
> + config->rows = 17;
> + mctl_core_init(para, config);
> +
> + /* store data again as it might be moved */
> + memcpy(buffer, (u32 *)CFG_SYS_SDRAM_BASE, sizeof(buffer));
> +
> + mctl_write_pattern();
> +
> + /* detect row address bits */
> + shift = config->bus_full_width + 4 + config->cols;
> + found = 0;
> + for (rows = 13; rows < 17; rows++) {
> + if (mctl_check_pattern(1ULL << (rows + shift))) {
> + found = 1;
> + break;
> + }
> + }
> + if (!found)
> + panic("DRAM init failed: Can't detect number of rows!");
> + debug("detected %u rows\n", rows);
> +
> + /* restore data again */
> + memcpy((u32 *)CFG_SYS_SDRAM_BASE, buffer, sizeof(buffer));
> +
> + config->cols = cols;
> + config->rows = rows;
> +}
> +
> +unsigned long mctl_calc_size(const struct dram_config *config)
> +{
> + u8 width = config->bus_full_width ? 4 : 2;
> +
> + /* 8 banks */
> + return (1ULL << (config->cols + config->rows + 3)) * width *
> config->ranks;
> +}
> diff --git a/arch/arm/mach-sunxi/dram_sun50i_h6.c
> b/arch/arm/mach-sunxi/dram_sun50i_h6.c
> index 6a9e53f965eb..fbb865131e08 100644
> --- a/arch/arm/mach-sunxi/dram_sun50i_h6.c
> +++ b/arch/arm/mach-sunxi/dram_sun50i_h6.c
> @@ -10,6 +10,7 @@
> #include <asm/io.h>
> #include <asm/arch/clock.h>
> #include <asm/arch/dram.h>
> +#include <asm/arch/dram_dw_helpers.h>
> #include <asm/arch/cpu.h>
> #include <asm/arch/prcm.h>
> #include <linux/bitops.h>
> @@ -40,8 +41,8 @@ static void mctl_com_init(const struct dram_para *para,
> static bool mctl_channel_init(const struct dram_para *para,
> const struct dram_config *config);
>
> -static bool mctl_core_init(const struct dram_para *para,
> - const struct dram_config *config)
> +bool mctl_core_init(const struct dram_para *para,
> + const struct dram_config *config)
> {
> mctl_sys_init(para->clk);
> mctl_com_init(para, config);
> @@ -560,92 +561,6 @@ static bool mctl_channel_init(const struct dram_para
> *para,
> return true;
> }
>
> -static void mctl_auto_detect_rank_width(const struct dram_para *para,
> - struct dram_config *config)
> -{
> - /* this is minimum size that it's supported */
> - config->cols = 8;
> - config->rows = 13;
> -
> - /*
> - * Previous versions of this driver tried to auto detect the rank
> - * and width by looking at controller registers. However this proved
> - * to be not reliable, so this approach here is the more robust
> - * solution. Check the git history for details.
> - *
> - * Strategy here is to test most demanding combination first and least
> - * demanding last, otherwise HW might not be fully utilized. For
> - * example, half bus width and rank = 1 combination would also work
> - * on HW with full bus width and rank = 2, but only 1/4 RAM would be
> - * visible.
> - */
> -
> - debug("testing 32-bit width, rank = 2\n");
> - config->bus_full_width = 1;
> - config->ranks = 2;
> - if (mctl_core_init(para, config))
> - return;
> -
> - debug("testing 32-bit width, rank = 1\n");
> - config->bus_full_width = 1;
> - config->ranks = 1;
> - if (mctl_core_init(para, config))
> - return;
> -
> - debug("testing 16-bit width, rank = 2\n");
> - config->bus_full_width = 0;
> - config->ranks = 2;
> - if (mctl_core_init(para, config))
> - return;
> -
> - debug("testing 16-bit width, rank = 1\n");
> - config->bus_full_width = 0;
> - config->ranks = 1;
> - if (mctl_core_init(para, config))
> - return;
> -
> - panic("This DRAM setup is currently not supported.\n");
> -}
> -
> -static void mctl_auto_detect_dram_size(const struct dram_para *para,
> - struct dram_config *config)
> -{
> - /* TODO: non-(LP)DDR3 */
> -
> - /* detect row address bits */
> - config->cols = 8;
> - config->rows = 18;
> - mctl_core_init(para, config);
> -
> - for (config->rows = 13; config->rows < 18; config->rows++) {
> - /* 8 banks, 8 bit per byte and 16/32 bit width */
> - if (mctl_mem_matches((1 << (config->rows + config->cols +
> - 4 + config->bus_full_width))))
> - break;
> - }
> -
> - /* detect column address bits */
> - config->cols = 11;
> - mctl_core_init(para, config);
> -
> - for (config->cols = 8; config->cols < 11; config->cols++) {
> - /* 8 bits per byte and 16/32 bit width */
> - if (mctl_mem_matches(1 << (config->cols + 1 +
> - config->bus_full_width)))
> - break;
> - }
> -}
> -
> -unsigned long mctl_calc_size(const struct dram_config *config)
> -{
> - u8 width = config->bus_full_width ? 4 : 2;
> -
> - /* TODO: non-(LP)DDR3 */
> -
> - /* 8 banks */
> - return (1ULL << (config->cols + config->rows + 3)) * width *
> config->ranks;
> -}
> -
> #define SUN50I_H6_LPDDR3_DX_WRITE_DELAYS \
> {{ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }, \
> { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }, \
> diff --git a/arch/arm/mach-sunxi/dram_sun50i_h616.c
> b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> index d1768a7e7d3a..80d9de557876 100644
> --- a/arch/arm/mach-sunxi/dram_sun50i_h616.c
> +++ b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> @@ -17,6 +17,7 @@
> #include <asm/io.h>
> #include <asm/arch/clock.h>
> #include <asm/arch/dram.h>
> +#include <asm/arch/dram_dw_helpers.h>
> #include <asm/arch/cpu.h>
> #include <asm/arch/prcm.h>
> #include <linux/bitops.h>
> @@ -1310,164 +1311,14 @@ static bool mctl_ctrl_init(const struct dram_para
> *para,
> return true;
> }
>
> -static bool mctl_core_init(const struct dram_para *para,
> - const struct dram_config *config)
> +bool mctl_core_init(const struct dram_para *para,
> + const struct dram_config *config)
> {
> mctl_sys_init(para->clk);
>
> return mctl_ctrl_init(para, config);
> }
>
> -static void mctl_auto_detect_rank_width(const struct dram_para *para,
> - struct dram_config *config)
> -{
> - /* this is minimum size that it's supported */
> - config->cols = 8;
> - config->rows = 13;
> -
> - /*
> - * Strategy here is to test most demanding combination first and least
> - * demanding last, otherwise HW might not be fully utilized. For
> - * example, half bus width and rank = 1 combination would also work
> - * on HW with full bus width and rank = 2, but only 1/4 RAM would be
> - * visible.
> - */
> -
> - debug("testing 32-bit width, rank = 2\n");
> - config->bus_full_width = 1;
> - config->ranks = 2;
> - if (mctl_core_init(para, config))
> - return;
> -
> - debug("testing 32-bit width, rank = 1\n");
> - config->bus_full_width = 1;
> - config->ranks = 1;
> - if (mctl_core_init(para, config))
> - return;
> -
> - debug("testing 16-bit width, rank = 2\n");
> - config->bus_full_width = 0;
> - config->ranks = 2;
> - if (mctl_core_init(para, config))
> - return;
> -
> - debug("testing 16-bit width, rank = 1\n");
> - config->bus_full_width = 0;
> - config->ranks = 1;
> - if (mctl_core_init(para, config))
> - return;
> -
> - panic("This DRAM setup is currently not supported.\n");
> -}
> -
> -static void mctl_write_pattern(void)
> -{
> - unsigned int i;
> - u32 *ptr, val;
> -
> - ptr = (u32 *)CFG_SYS_SDRAM_BASE;
> - for (i = 0; i < 16; ptr++, i++) {
> - if (i & 1)
> - val = ~(ulong)ptr;
> - else
> - val = (ulong)ptr;
> - writel(val, ptr);
> - }
> -}
> -
> -static bool mctl_check_pattern(ulong offset)
> -{
> - unsigned int i;
> - u32 *ptr, val;
> -
> - ptr = (u32 *)CFG_SYS_SDRAM_BASE;
> - for (i = 0; i < 16; ptr++, i++) {
> - if (i & 1)
> - val = ~(ulong)ptr;
> - else
> - val = (ulong)ptr;
> - if (val != *(ptr + offset / 4))
> - return false;
> - }
> -
> - return true;
> -}
> -
> -static void mctl_auto_detect_dram_size(const struct dram_para *para,
> - struct dram_config *config)
> -{
> - unsigned int shift, cols, rows, found;
> - u32 buffer[16];
> -
> - /* max. config for columns, but not rows */
> - config->cols = 11;
> - config->rows = 13;
> - mctl_core_init(para, config);
> -
> - /*
> - * Store content so it can be restored later. This is important
> - * if controller was already initialized and holds any data
> - * which is important for restoring system.
> - */
> - memcpy(buffer, (u32 *)CFG_SYS_SDRAM_BASE, sizeof(buffer));
> -
> - mctl_write_pattern();
> -
> - shift = config->bus_full_width + 1;
> -
> - /* detect column address bits */
> - found = 0;
> - for (cols = 8; cols < 11; cols++) {
> - if (mctl_check_pattern(1ULL << (cols + shift))) {
> - found = 1;
> - break;
> - }
> - }
> - if (!found)
> - panic("DRAM init failed: Can't detect number of columns!");
> - debug("detected %u columns\n", cols);
> -
> - /* restore data */
> - memcpy((u32 *)CFG_SYS_SDRAM_BASE, buffer, sizeof(buffer));
> -
> - /* reconfigure to make sure that all active rows are accessible */
> - config->cols = 8;
> - config->rows = 17;
> - mctl_core_init(para, config);
> -
> - /* store data again as it might be moved */
> - memcpy(buffer, (u32 *)CFG_SYS_SDRAM_BASE, sizeof(buffer));
> -
> - mctl_write_pattern();
> -
> - /* detect row address bits */
> - shift = config->bus_full_width + 4 + config->cols;
> - found = 0;
> - for (rows = 13; rows < 17; rows++) {
> - if (mctl_check_pattern(1ULL << (rows + shift))) {
> - found = 1;
> - break;
> - }
> - }
> - if (!found)
> - panic("DRAM init failed: Can't detect number of rows!");
> - debug("detected %u rows\n", rows);
> -
> - /* restore data again */
> - memcpy((u32 *)CFG_SYS_SDRAM_BASE, buffer, sizeof(buffer));
> -
> - config->cols = cols;
> - config->rows = rows;
> -}
> -
> -static unsigned long mctl_calc_size(const struct dram_config *config)
> -{
> - u8 width = config->bus_full_width ? 4 : 2;
> -
> - /* 8 banks */
> - return (1ULL << (config->cols + config->rows + 3)) * width *
> config->ranks;
> -}
> -
> static const struct dram_para para = {
> .clk = CONFIG_DRAM_CLK,
> #ifdef CONFIG_SUNXI_DRAM_H616_DDR3_1333
