Re: [PATCH 30/34] sunxi: A523: add DRAM initialisation routine

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

On Sat, 5 Apr 2025 22:01:50 +0000
Yixun Lan <dlan@gentoo.org> wrote:

Hi,

> Hi Andre:
> 
> On 11:35 Sun 23 Mar     , Andre Przywara wrote:
> > From: Jernej Skrabec <jernej.skrabec@gmail.com>
> > 
> > DRAM init code, as per reverse engineering and matching against
> > previous SoCs.
> > Supports LPDDR4 for now only.
> > ---
> >  arch/arm/include/asm/arch-sunxi/dram.h        |    2 +
> >  .../include/asm/arch-sunxi/dram_sun55i_a523.h |  183 ++
> >  arch/arm/mach-sunxi/Kconfig                   |   31 +-
> >  arch/arm/mach-sunxi/Makefile                  |    2 +
> >  arch/arm/mach-sunxi/dram_sun55i_a523.c        | 1468 +++++++++++++++++
> >  arch/arm/mach-sunxi/dram_timings/Makefile     |    1 +
> >  .../arm/mach-sunxi/dram_timings/a523_lpddr4.c |  119 ++
> >  7 files changed, 1804 insertions(+), 2 deletions(-)
> >  create mode 100644 arch/arm/include/asm/arch-sunxi/dram_sun55i_a523.h
> >  create mode 100644 arch/arm/mach-sunxi/dram_sun55i_a523.c
> >  create mode 100644 arch/arm/mach-sunxi/dram_timings/a523_lpddr4.c
> > 
[ ... ]

> > diff --git a/arch/arm/mach-sunxi/dram_sun55i_a523.c b/arch/arm/mach-sunxi/dram_sun55i_a523.c
> > new file mode 100644
> > index 00000000000..fae02062547
> > --- /dev/null
> > +++ b/arch/arm/mach-sunxi/dram_sun55i_a523.c
> > @@ -0,0 +1,1468 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * sun55i A523/A527/T527/H728 platform DRAM controller driver
> > + *
> > + * This driver supports DDR3 and LPDDR4 memory.
> > + *
> > + * (C) Copyright 2024 Jernej Skrabec <jernej.skrabec@gmail.com>
> > + *
> > + */
> > +#include <init.h>
> > +#include <log.h>
> > +#include <asm/io.h>
> > +#include <asm/arch/clock.h>
> > +#include <asm/arch/dram.h>
> > +#include <asm/arch/cpu.h>
> > +#include <asm/arch/prcm.h>
> > +#include <linux/bitops.h>
> > +#include <linux/delay.h>
> > +  
> ...snip
> > +static void mctl_auto_detect_dram_size(const struct dram_para *para,
> > +				       struct dram_config *config)
> > +{
> > +	/* detect row address bits */
> > +	config->cols = 8;
> > +	config->rows = 16;
> > +	mctl_core_init(para, config);
> > +
> > +	for (config->rows = 13; config->rows < 16; 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;
> > +	}  
> on radxa a5e, I've got occasionally wrong dram size, roughly 2/10 rate
> in the wrong case it got 8192M, while actually should be 4096M..
> 
> spent a few time to debug, found it got config->cols = 11 while should be 10
> and above for loop has been skipped, thus fail to detect correct cols value..

Ah yes, we were already suspecting that. We had those "double detection"
issues on H616 for a while, and Jernej fixed them there recently:
https://lore.kernel.org/u-boot/20250309063143.62859-1-jernej.skrabec@gmail.com/T/#u

So we were already thinking of folding a similar fix into this (and
other SoCs') DRAM code, ideally by sharing some code.

Thanks for the test and the report!

Cheers,
Andre