[PATCH] riscv: spacemit: k1: probe dram size during boot phase.

[Huan Zhou]

On Wed, Jan 08, 2025 at 03:37:37PM +0300, Dan Carpenter wrote:
> On Wed, Jan 08, 2025 at 12:21:18PM +0100, Heinrich Schuchardt wrote:
> > Am 8. Januar 2025 12:11:05 MEZ schrieb Yixun Lan <dlan at gentoo.org>:
> > >Hi Huan:
> > >
> > >On 16:49 Wed 08 Jan     , Huan Zhou wrote:
> > >> 
> > >> 
> > >..
> > >> ---
> > >remove above "---"? otherwise following commit message will be
> > >dropped during patch application..
> > >
> > >> This patch introduce improvement for get dram size on bananapi BPI-F3,
> > >> retrieving the dram size dynamically.
> > >> Have tested on bananapi BPIF3 4G and jupiter 8G.
> > >> 
> > >> Signed-off-by: Huan Zhou <me at per1cycle.org>
> > >> ---
> > >>  arch/riscv/cpu/k1/dram.c | 40 ++++++++++++++++++++++++++++++++++++++--
> > >>  1 file changed, 38 insertions(+), 2 deletions(-)
> > >> 
> > >> diff --git a/arch/riscv/cpu/k1/dram.c b/arch/riscv/cpu/k1/dram.c
> > >> index c477c15cbfb19f0e3a0ee72985b602f5bda352d7..095217f2a4c053f7477d62c0776bcb51e623db47 100644
> > >> --- a/arch/riscv/cpu/k1/dram.c
> > >> +++ b/arch/riscv/cpu/k1/dram.c
> > >> @@ -4,17 +4,53 @@
> > >>   */
> > >>  
> > >>  #include <asm/global_data.h>
> > >> +#include <asm/io.h>
> > >>  #include <config.h>
> > >> +#include <bitfield.h>
> > >>  #include <fdt_support.h>
> > >>  #include <linux/sizes.h>
> > >>  
> > >> +#define DDR_BASE 0xC0000000
> > >>  DECLARE_GLOBAL_DATA_PTR;
> > >>  
> > >> +static inline u32 map_format_size(u32 val)
> > >> +{
> > >> +	u32 tmp;
> > >> +
> > >> +	if (val & 0x1 == 0)
> > >please add brackets explicitly, something like
> > >	if ((val & 0x1) == 0)
> > 
> > We tend to avoid == 0 in U-Boot
> > 
> > if (val & BIT(0))
it should be (val & 0x1) == 0 here, i test that version of patch and do some simplification on the origin code..
but forgot test here, btw personally i would keep the (==0) here... 
> > 
> 
> That's reversed.
> 
> 	if (!(val & BIT(0)) {
> 
> I have really complicated rules about when to use ! vs == 0.
> https://staticthinking.wordpress.com/2024/02/20/when-to-use-0/
> 
> > >
> > >> +		return 0;
> > >> +
> > >> +	tmp = bitfield_extract(val, 16, 5);
> > >> +	switch (tmp) {
> > >> +	case 0xd:
> > >> +		return 512;
> > >> +	case 0xe:
> > >> +		return 1024;
> > >> +	case 0xf:
> > >> +		return 2048;
> > >> +	case 0x10:
> > >> +		return 4096;
> > >> +	case 0x11:
> > >> +		return 8192;
> > >> +	default:
> > >> +		pr_info("Invalid DRAM density %x\n", val);
> > >> +		return 0;
> > >> +	}
> > >> +}
> > >> +
> > >> +u32 ddr_get_density(void)
> > >> +{
> > >> +	u32 cs0_size = map_format_size(readl((void *)DDR_BASE + 0x200));
> > >> +	u32 cs1_size = map_format_size(readl((void *)DDR_BASE + 0x208));
> > >> +	u32 ddr_size = cs0_size + cs1_size;
> > >> +
> > >> +	return ddr_size;
> > >> +}
> > >> +
> > >>  int dram_init(void)
> > >>  {
> > >>  	gd->ram_base = CFG_SYS_SDRAM_BASE;
> > >> -	/* TODO get ram size from ddr controller */
> > >> -	gd->ram_size = SZ_4G;
> > >> +	gd->ram_size = (u64)ddr_get_density() * SZ_1M;
> > 
> > This is C and not C++. We don't need a cast to assign here.
> > 
> 
> I assumed the cast was to prevent an integer overflow.  The commit
> message mentions 8GB?  I agree with Yixun Lan that the type of
> ddr_get_density() should be fixed and probably SZ_1M should be declared
> as unsigned long as well.
ok, will fixed in next version, thx.
> 
> regards,
> dan carpenter
>