On Wed, Jan 08, 2025 at 12:21:18PM +0100, Heinrich Schuchardt wrote:
> Am 8. Januar 2025 12:11:05 MEZ schrieb Yixun Lan <d...@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 <m...@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))
>
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.
regards,
dan carpenter
