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

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

[Huan Zhou]

---
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@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)
+		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;
 	return 0;
 }
 

---
base-commit: 19fc0b7f7d907119a13e9c207991899f0817f8fc
change-id: 20250108-get-dram-size-65cf59a15201

Best regards,
-- 
Huan Zhou <me@per1cycle.org>

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

[Yixun Lan]

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@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)

> +		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;
while using cast, why not define ddr_get_density() return as same type?
also it's more reasonable to use phys_size_t as I checked gd_t

>  	return 0;
>  }
>  
> 
> ---
> base-commit: 19fc0b7f7d907119a13e9c207991899f0817f8fc
> change-id: 20250108-get-dram-size-65cf59a15201
> 
> Best regards,
> -- 
> Huan Zhou <me@per1cycle.org>
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

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

[Heinrich Schuchardt]

Am 8. Januar 2025 12:11:05 MEZ schrieb Yixun Lan <dlan@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@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))

>
>> +		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.

Best regards

Heinrich

>while using cast, why not define ddr_get_density() return as same type?
>also it's more reasonable to use phys_size_t as I checked gd_t
>
>>  	return 0;
>>  }
>>  
>> 
>> ---
>> base-commit: 19fc0b7f7d907119a13e9c207991899f0817f8fc
>> change-id: 20250108-get-dram-size-65cf59a15201
>> 
>> Best regards,
>> -- 
>> Huan Zhou <me@per1cycle.org>
>> 
>

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

[Huan Zhou]

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@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@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
ok, got it.
> 
> if (val & BIT(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.
> 
ok, will fixed and retested, links to test log will be added too.
> Best regards
> 
> Heinrich
> 
> >while using cast, why not define ddr_get_density() return as same type?
> >also it's more reasonable to use phys_size_t as I checked gd_t
> >
> >>  	return 0;
> >>  }
> >>  
> >> 
> >> ---
> >> base-commit: 19fc0b7f7d907119a13e9c207991899f0817f8fc
> >> change-id: 20250108-get-dram-size-65cf59a15201
> >> 
> >> Best regards,
> >> -- 
> >> Huan Zhou <me@per1cycle.org>
> >> 
> >
> 

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

[Dan Carpenter]

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@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@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

Re: [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@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@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
> 

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

[Simon Glass]

Hi Dan,

On Wed, 8 Jan 2025 at 05:37, Dan Carpenter <dan.carpenter@linaro.org> 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@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@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/

I  mostly agree with that and it provides some motivation for the
conventions which have built up over the years in Linux/U-Boot/etc.

If you want to send a patch:

$ git grep 'if (ret != EFI_SUCCESS)' |wc
    864    4841   51380

I think the only place I would differ is with strcmp(), where I've got
used to 0 meaning success, like in much other code.

[..]

Regards,
SImon

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]

On Wed, Jan 08, 2025 at 10:03:08AM -0700, Simon Glass wrote:
> Hi Dan,
> 
> On Wed, 8 Jan 2025 at 05:37, Dan Carpenter <dan.carpenter@linaro.org> 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@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@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/
> 
> I  mostly agree with that and it provides some motivation for the
> conventions which have built up over the years in Linux/U-Boot/etc.
> 
> If you want to send a patch:
> 
> $ git grep 'if (ret != EFI_SUCCESS)' |wc
>     864    4841   51380
> 
> I think the only place I would differ is with strcmp(), where I've got
> used to 0 meaning success, like in much other code.

To be clear here, we have ~900 examples of EFI_SUCCESS and ~250 examples
of other FOO_SUCCESS type tests. And if we're going to make some sort of
change here, we should (a) document it and (b) fix it everywhere.

And at the risk of confusing your intentions Simon, Dan has a well
deserved reputation in the Linux kernel for fixing what I would call "C
is trickier than you think, even if you're been doing it for decades"
bugs so just changing EFI code would be silly.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Dan Carpenter]

On Wed, Jan 08, 2025 at 10:03:08AM -0700, Simon Glass wrote:
> Hi Dan,
> 
> On Wed, 8 Jan 2025 at 05:37, Dan Carpenter <dan.carpenter@linaro.org> 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@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@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/
> 
> I  mostly agree with that and it provides some motivation for the
> conventions which have built up over the years in Linux/U-Boot/etc.
> 
> If you want to send a patch:
> 
> $ git grep 'if (ret != EFI_SUCCESS)' |wc
>     864    4841   51380
> 
> I think the only place I would differ is with strcmp(), where I've got
> used to 0 meaning success, like in much other code.
> 

Huh...  I've never thought about it as a success/fail thing.

I feel like I've seens quite a few reversed strcmp() functions but it's
hard to catch these sorts of bugs with static analysis because it
totally depends on the context if you want the strings to be equal or
not equal.  I can't imagine what a static checker rule would look like.

regards,
dan carpenter

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

[Simon Glass]

Hi Dan,

On Wed, 8 Jan 2025 at 10:18, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Wed, Jan 08, 2025 at 10:03:08AM -0700, Simon Glass wrote:
> > Hi Dan,
> >
> > On Wed, 8 Jan 2025 at 05:37, Dan Carpenter <dan.carpenter@linaro.org> 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@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@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/
> >
> > I  mostly agree with that and it provides some motivation for the
> > conventions which have built up over the years in Linux/U-Boot/etc.
> >
> > If you want to send a patch:
> >
> > $ git grep 'if (ret != EFI_SUCCESS)' |wc
> >     864    4841   51380
> >
> > I think the only place I would differ is with strcmp(), where I've got
> > used to 0 meaning success, like in much other code.
> >
>
> Huh...  I've never thought about it as a success/fail thing.

I have evolved on that one.

>
> I feel like I've seens quite a few reversed strcmp() functions but it's
> hard to catch these sorts of bugs with static analysis because it
> totally depends on the context if you want the strings to be equal or
> not equal.  I can't imagine what a static checker rule would look like.

There's no substitute for tests and naming is hard. We could have a
streq() function which just returns 0 or 1, but it would presumably
have to return 1/true when equal, thus creating confusion with
strcmp(). Perhaps strcheckeq() would be a better name for a function
that returns '0' on success?

Regards,
Simon

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

[Huan Zhou]

On Wed, Jan 08, 2025 at 07:11:05PM +0800, Yixun Lan wrote:
> 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@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)
ok, mb, will correct and test in next version.
> 
> > +		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;
> while using cast, why not define ddr_get_density() return as same type?
> also it's more reasonable to use phys_size_t as I checked gd_t
ok, got it.
> 
> >  	return 0;
> >  }
> >  
> > 
> > ---
> > base-commit: 19fc0b7f7d907119a13e9c207991899f0817f8fc
> > change-id: 20250108-get-dram-size-65cf59a15201
> > 
> > Best regards,
> > -- 
> > Huan Zhou <me@per1cycle.org>
> > 
> 
> -- 
> Yixun Lan (dlan)
> Gentoo Linux Developer
> GPG Key ID AABEFD55

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

[Huan Zhou]

On Wed, Jan 08, 2025 at 07:11:05PM +0800, Yixun Lan wrote:
> Hi Huan:
> 
> On 16:49 Wed 08 Jan     , Huan Zhou wrote:
> > 
> > 
> ..
> > ---
> remove above "---"? otherwise following commit message will be
> dropped during patch application..
> 
ill have a check.
> > 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@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)
ok, sry for this dummy mistake ;(
> 
> > +		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;
this two line not sure why it appear, also will be removed in next version
> > -	/* TODO get ram size from ddr controller */
> > -	gd->ram_size = SZ_4G;

> > +	gd->ram_size = (u64)ddr_get_density() * SZ_1M;
> while using cast, why not define ddr_get_density() return as same type?
> also it's more reasonable to use phys_size_t as I checked gd_t
ok, will fixed in next version.
> 
> >  	return 0;
> >  }
> >  
> > 
> > ---
> > base-commit: 19fc0b7f7d907119a13e9c207991899f0817f8fc
> > change-id: 20250108-get-dram-size-65cf59a15201
> > 
> > Best regards,
> > -- 
> > Huan Zhou <me@per1cycle.org>
> > 
> 
> -- 
> Yixun Lan (dlan)
> Gentoo Linux Developer
> GPG Key ID AABEFD55

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

[Marcel Ziswiler]

Hi Huan Zhou

Thanks!

On Wed, 2025-01-08 at 16:49 +0800, Huan Zhou wrote:
> 
> ---
> 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@per1cycle.org>

Tested-by: Marcel Ziswiler <marcel@ziswiler.com> # BPI-F3 16G

> ---
>  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)
> +		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;
>  	return 0;
>  }
>  
> 
> ---
> base-commit: 19fc0b7f7d907119a13e9c207991899f0817f8fc
> change-id: 20250108-get-dram-size-65cf59a15201
> 
> Best regards,
> --
> Huan Zhou <me@per1cycle.org>

Cheers

Marcel