[U-Boot] [PATCH 0/9] Support 4GB of memory on 32bit systems

[U-Boot] [PATCH 0/9] Support 4GB of memory on 32bit systems

[Philipp Tomsich]

Even on 32bit systems a full 4GB of DRAM may be installed and reported
by the DRAM controller.  Whether these 4GB are larger available
depends on the size/configuration of address decoding windows and
architectural features.

This increases the fields holding the RAM size to have at least 33bits
(i.e. we use a u64) and fixes the fallout from the change: some casts,
the usage of min() and a few printf formats have to be adjusted.


Philipp Tomsich (9):
  dm: allow 4GB of DRAM on 32bit systems
  rockchip: support 4GB DRAM on 32bit systems
  common: include <inttypes.h> always
  MIPS: use PRIx64 macros for printing ram size
  rockchip: rk3368: change type of ram-size field for a
    min()-calculation
  ram: stm32mp1: use PRIx64 macros for printing ram size
  board: keymile: add explicit cast to truncate the 64bit ram size field
  board: cm_fx6: use PRIx64 macros for printing ram size
  mpc85xx: add casts for ram size in min() calculation.

 arch/arm/include/asm/arch-rockchip/sdram_common.h | 2 +-
 arch/arm/mach-rockchip/sdram_common.c             | 8 ++++----
 arch/arm/mach-stm32mp/dram_init.c                 | 2 +-
 arch/mips/mach-bmips/dram.c                       | 2 +-
 arch/powerpc/cpu/mpc85xx/cpu.c                    | 6 ++++--
 board/compulab/cm_fx6/cm_fx6.c                    | 3 ++-
 board/keymile/km_arm/km_arm.c                     | 6 ++++--
 drivers/ram/rockchip/dmc-rk3368.c                 | 2 +-
 drivers/ram/stm32mp1/stm32mp1_ram.c               | 2 +-
 include/asm-generic/global_data.h                 | 2 +-
 include/common.h                                  | 1 +
 include/ram.h                                     | 9 ++++++++-
 12 files changed, 29 insertions(+), 16 deletions(-)

-- 
2.1.4

[U-Boot] [PATCH 1/9] dm: allow 4GB of DRAM on 32bit systems

[Philipp Tomsich]

Even on 32bit systems a full 4GB of DRAM may be installed and reported
by the DRAM controller.  Whether these 4GB are larger available
depends on the size/configuration of address decoding windows and
architectural features (e.g. consider a hypothetical architecture that
uses dedicated instructions to access the 'memory-mapped' peripheral
IO ranges).  In U-Boot, the available DRAM, as reported by the
device-model is independent of the accessible DRAM (i.e. adjusted top
and effective size).

This was first reported against the RK3288, which may have 4GB DRAM
attached, but will have a small (e.g. 128MB) window not accessible due
to address decoding limitations.

The solution is to increase the types used for storing the ram_size to
have at least 33 bits even on 32bit systems... i.e. we need to use a
u64 for these always (this was previously only the case for
PHYS_64BIT, which will have unwanted side-effects for 32bit systems
and would require more intrusive changes).

This commit changes the size-field in 'struct ram' and the ram_size in
'gd' to be a u64.

Reported-by: Marty E. Plummer <hanetzer@startmail.com>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 include/asm-generic/global_data.h | 2 +-
 include/ram.h                     | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index 0fd4900..f3d687c 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -55,7 +55,7 @@ typedef struct global_data {
 	unsigned long ram_base;		/* Base address of RAM used by U-Boot */
 	unsigned long ram_top;		/* Top address of RAM used by U-Boot */
 	unsigned long relocaddr;	/* Start address of U-Boot in RAM */
-	phys_size_t ram_size;		/* RAM size */
+	u64 ram_size;			/* RAM size */
 	unsigned long mon_len;		/* monitor len */
 	unsigned long irq_sp;		/* irq stack pointer */
 	unsigned long start_addr_sp;	/* start_addr_stackpointer */
diff --git a/include/ram.h b/include/ram.h
index 67e22d7..1fe024f 100644
--- a/include/ram.h
+++ b/include/ram.h
@@ -9,7 +9,14 @@
 
 struct ram_info {
 	phys_addr_t base;
-	size_t size;
+	/*
+	 * We use a 64bit type for the size to correctly handle 32bit
+	 * systems with 4GB of DRAM (which would overflow a 32bit type
+	 * and read back as 0).  For 64bit systems, we assume (for now)
+	 * that they will always have less than 2^65 byte of DRAM
+	 * installed. -- prt
+	 */
+	u64 size;
 };
 
 struct ram_ops {
-- 
2.1.4

[U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems

[Philipp Tomsich]

The calculation in `rockchip_sdram_size` would overflow for 4GB on
32bit systems (i.e. when PHYS_64BIT is not defined).

This makes the internal variables and the signature prototype use a
u64 to ensure that we can represent the 33rd bit (as in '4GB').

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 arch/arm/include/asm/arch-rockchip/sdram_common.h | 2 +-
 arch/arm/mach-rockchip/sdram_common.c             | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/arch-rockchip/sdram_common.h b/arch/arm/include/asm/arch-rockchip/sdram_common.h
index 671c318..edf5911 100644
--- a/arch/arm/include/asm/arch-rockchip/sdram_common.h
+++ b/arch/arm/include/asm/arch-rockchip/sdram_common.h
@@ -50,7 +50,7 @@
 #define SYS_REG_DBW_MASK		3
 
 /* Get sdram size decode from reg */
-size_t rockchip_sdram_size(phys_addr_t reg);
+u64 rockchip_sdram_size(phys_addr_t reg);
 
 /* Called by U-Boot board_init_r for Rockchip SoCs */
 int dram_init(void);
diff --git a/arch/arm/mach-rockchip/sdram_common.c b/arch/arm/mach-rockchip/sdram_common.c
index 650d53e..6bad537 100644
--- a/arch/arm/mach-rockchip/sdram_common.c
+++ b/arch/arm/mach-rockchip/sdram_common.c
@@ -11,11 +11,11 @@
 #include <dm/uclass-internal.h>
 
 DECLARE_GLOBAL_DATA_PTR;
-size_t rockchip_sdram_size(phys_addr_t reg)
+u64 rockchip_sdram_size(phys_addr_t reg)
 {
 	u32 rank, col, bk, cs0_row, cs1_row, bw, row_3_4;
-	size_t chipsize_mb = 0;
-	size_t size_mb = 0;
+	u64 chipsize_mb = 0;
+	u64 size_mb = 0;
 	u32 ch;
 
 	u32 sys_reg = readl(reg);
@@ -48,7 +48,7 @@ size_t rockchip_sdram_size(phys_addr_t reg)
 		      rank, col, bk, cs0_row, bw, row_3_4);
 	}
 
-	return (size_t)size_mb << 20;
+	return (u64)size_mb << 20;
 }
 
 int dram_init(void)
-- 
2.1.4

[U-Boot] [PATCH 3/9] common: include <inttypes.h> always

[Philipp Tomsich]

With the ram-size variable changed to u64, we'll need appropriate
macros for printing u64 values correctly either in 32bit builds
(i.e. ILP32 models) or in 64bit builds (i.e. LP64 models).  Best to
make the PRIx64 macro available everywhere.

This include inttypes.h from common.h to make the various macros for
formatted printing available to everyone.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 include/common.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/common.h b/include/common.h
index 940161f..9de9145 100644
--- a/include/common.h
+++ b/include/common.h
@@ -30,6 +30,7 @@ typedef volatile unsigned char	vu_char;
 #include <linux/stringify.h>
 #include <asm/ptrace.h>
 #include <stdarg.h>
+#include <inttypes.h>
 #include <stdio.h>
 #include <linux/kernel.h>
 
-- 
2.1.4

[U-Boot] [PATCH 4/9] MIPS: use PRIx64 macros for printing ram size

[Philipp Tomsich]

With the recent changes of the underlying types for the ram size, we need
to adjust the formatting.  Use PRIx64 to print the (now) u64 field.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 arch/mips/mach-bmips/dram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/mach-bmips/dram.c b/arch/mips/mach-bmips/dram.c
index 87ced7c..0884bce 100644
--- a/arch/mips/mach-bmips/dram.c
+++ b/arch/mips/mach-bmips/dram.c
@@ -28,7 +28,7 @@ int dram_init(void)
 		return 0;
 	}
 
-	debug("SDRAM base=%zx, size=%x\n", ram.base, ram.size);
+	debug("SDRAM base=%zx, size=%" PRIx64 "\n", ram.base, ram.size);
 
 	gd->ram_size = ram.size;
 
-- 
2.1.4

[U-Boot] [PATCH 5/9] rockchip: rk3368: change type of ram-size field for a min()-calculation

[Philipp Tomsich]

With the recent changes of the underlying types for the ram size, we need
to cast the constant used in clamping to u64 instead of size_t.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 drivers/ram/rockchip/dmc-rk3368.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ram/rockchip/dmc-rk3368.c b/drivers/ram/rockchip/dmc-rk3368.c
index 8d1b9fa..35d7605 100644
--- a/drivers/ram/rockchip/dmc-rk3368.c
+++ b/drivers/ram/rockchip/dmc-rk3368.c
@@ -960,7 +960,7 @@ static int rk3368_dmc_probe(struct udevice *dev)
 	* is SoC register space (i.e. reserved), and 0xfe000000~0xfeffffff is
 	* inaccessible for some IP controller.
 	*/
-	priv->info.size = min(priv->info.size, (size_t)0xfe000000);
+	priv->info.size = min(priv->info.size, (u64)0xfe000000);
 
 	return 0;
 }
-- 
2.1.4

[U-Boot] [PATCH 6/9] ram: stm32mp1: use PRIx64 macros for printing ram size

[Philipp Tomsich]

With the recent changes of the underlying types for the ram size, we
need to adjust the formatting.  Use PRIx64 to print the (now) u64
field.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 arch/arm/mach-stm32mp/dram_init.c   | 2 +-
 drivers/ram/stm32mp1/stm32mp1_ram.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-stm32mp/dram_init.c b/arch/arm/mach-stm32mp/dram_init.c
index 7688b3e..e4c6302 100644
--- a/arch/arm/mach-stm32mp/dram_init.c
+++ b/arch/arm/mach-stm32mp/dram_init.c
@@ -25,7 +25,7 @@ int dram_init(void)
 		debug("Cannot get RAM size: %d\n", ret);
 		return ret;
 	}
-	debug("RAM init base=%lx, size=%x\n", ram.base, ram.size);
+	debug("RAM init base=%lx, size=%" PRIx64 "\n", ram.base, ram.size);
 
 	gd->ram_size = ram.size;
 
diff --git a/drivers/ram/stm32mp1/stm32mp1_ram.c b/drivers/ram/stm32mp1/stm32mp1_ram.c
index bd497a3..faf78b2 100644
--- a/drivers/ram/stm32mp1/stm32mp1_ram.c
+++ b/drivers/ram/stm32mp1/stm32mp1_ram.c
@@ -130,7 +130,7 @@ static __maybe_unused int stm32mp1_ddr_setup(struct udevice *dev)
 
 	/* check memory access for all memory */
 	if (config.info.size != priv->info.size) {
-		printf("DDR invalid size : 0x%x, expected 0x%x\n",
+		printf("DDR invalid size : 0x%" PRIx64 ", expected 0x%x\n",
 		       priv->info.size, config.info.size);
 		return -EINVAL;
 	}
-- 
2.1.4

[U-Boot] [PATCH 7/9] board: keymile: add explicit cast to truncate the 64bit ram size field

[Philipp Tomsich]

With the change to a 64bit ram size field for 32bit architectures, we need
to add an explicit cast to truncate when converting to a pointer.
This casts to 'void*' through 'uintptr_t'.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 board/keymile/km_arm/km_arm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/board/keymile/km_arm/km_arm.c b/board/keymile/km_arm/km_arm.c
index ea03be9..13304cf 100644
--- a/board/keymile/km_arm/km_arm.c
+++ b/board/keymile/km_arm/km_arm.c
@@ -521,13 +521,15 @@ int post_hotkeys_pressed(void)
 
 ulong post_word_load(void)
 {
-	void* addr = (void *) (gd->ram_size - BOOTCOUNT_ADDR + POST_WORD_OFF);
+	void *addr = (void *)((uintptr_t)gd->ram_size -
+			      BOOTCOUNT_ADDR + POST_WORD_OFF);
 	return in_le32(addr);
 
 }
 void post_word_store(ulong value)
 {
-	void* addr = (void *) (gd->ram_size - BOOTCOUNT_ADDR + POST_WORD_OFF);
+	void *addr = (void *)((uintptr_t)gd->ram_size -
+			      BOOTCOUNT_ADDR + POST_WORD_OFF);
 	out_le32(addr, value);
 }
 
-- 
2.1.4

[U-Boot] [PATCH 8/9] board: cm_fx6: use PRIx64 macros for printing ram size

[Philipp Tomsich]

With the recent changes of the underlying types for the ram size, we
need to adjust the formatting.  Use PRIx64 to print the (now) u64
field.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 board/compulab/cm_fx6/cm_fx6.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
index c114cdc..b163abf 100644
--- a/board/compulab/cm_fx6/cm_fx6.c
+++ b/board/compulab/cm_fx6/cm_fx6.c
@@ -704,7 +704,8 @@ int dram_init(void)
 		gd->ram_size -= 0x100000;
 		break;
 	default:
-		printf("ERROR: Unsupported DRAM size 0x%lx\n", gd->ram_size);
+		printf("ERROR: Unsupported DRAM size 0x%" PRIx64 "\n",
+		       gd->ram_size);
 		return -1;
 	}
 
-- 
2.1.4

[U-Boot] [PATCH 9/9] mpc85xx: add casts for ram size in min() calculation.

[Philipp Tomsich]

With the change of the ram size fields to u64, we need to use explicit
casts to ensure that min()-implementation sees the same type for both
its arguments.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 arch/powerpc/cpu/mpc85xx/cpu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/cpu.c b/arch/powerpc/cpu/mpc85xx/cpu.c
index bf48836..5664872 100644
--- a/arch/powerpc/cpu/mpc85xx/cpu.c
+++ b/arch/powerpc/cpu/mpc85xx/cpu.c
@@ -604,7 +604,8 @@ static int reset_tlb(phys_addr_t p_addr, u32 size, phys_addr_t *phys_offset)
 int arch_memory_test_advance(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
 {
 	phys_addr_t test_cap, p_addr;
-	phys_size_t p_size = min(gd->ram_size, CONFIG_MAX_MEM_MAPPED);
+	phys_size_t p_size = min((phys_size_t)gd->ram_size,
+				 (phys_size_t)CONFIG_MAX_MEM_MAPPED);
 
 #if !defined(CONFIG_PHYS_64BIT) || \
     !defined(CONFIG_SYS_INIT_RAM_ADDR_PHYS) || \
@@ -632,7 +633,8 @@ int arch_memory_test_advance(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
 /* initialization for testing area */
 int arch_memory_test_prepare(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
 {
-	phys_size_t p_size = min(gd->ram_size, CONFIG_MAX_MEM_MAPPED);
+	phys_size_t p_size = min((phys_size_t)gd->ram_size,
+				 (phys_size_t)CONFIG_MAX_MEM_MAPPED);
 
 	*vstart = CONFIG_SYS_DDR_SDRAM_BASE;
 	*size = (u32) p_size;	/* CONFIG_MAX_MEM_MAPPED < 4G */
-- 
2.1.4

[U-Boot] [PATCH 9/9] mpc85xx: add casts for ram size in min() calculation.

[York Sun]

On 07/26/2018 07:00 AM, Philipp Tomsich wrote:
> With the change of the ram size fields to u64, we need to use explicit
> casts to ensure that min()-implementation sees the same type for both
> its arguments.
> 
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
> 
>  arch/powerpc/cpu/mpc85xx/cpu.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/cpu/mpc85xx/cpu.c b/arch/powerpc/cpu/mpc85xx/cpu.c
> index bf48836..5664872 100644
> --- a/arch/powerpc/cpu/mpc85xx/cpu.c
> +++ b/arch/powerpc/cpu/mpc85xx/cpu.c
> @@ -604,7 +604,8 @@ static int reset_tlb(phys_addr_t p_addr, u32 size, phys_addr_t *phys_offset)
>  int arch_memory_test_advance(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
>  {
>  	phys_addr_t test_cap, p_addr;
> -	phys_size_t p_size = min(gd->ram_size, CONFIG_MAX_MEM_MAPPED);
> +	phys_size_t p_size = min((phys_size_t)gd->ram_size,
> +				 (phys_size_t)CONFIG_MAX_MEM_MAPPED);
>  
>  #if !defined(CONFIG_PHYS_64BIT) || \
>      !defined(CONFIG_SYS_INIT_RAM_ADDR_PHYS) || \
> @@ -632,7 +633,8 @@ int arch_memory_test_advance(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
>  /* initialization for testing area */
>  int arch_memory_test_prepare(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
>  {
> -	phys_size_t p_size = min(gd->ram_size, CONFIG_MAX_MEM_MAPPED);
> +	phys_size_t p_size = min((phys_size_t)gd->ram_size,
> +				 (phys_size_t)CONFIG_MAX_MEM_MAPPED);
>  
>  	*vstart = CONFIG_SYS_DDR_SDRAM_BASE;
>  	*size = (u32) p_size;	/* CONFIG_MAX_MEM_MAPPED < 4G */
> 

We didn't have any issue with 36-bit physical address on mpc85xx. I see
this change is needed after you change gd->ram_size.

Reviewed-by: York Sun <york.sun@nxp.com>

[U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems

[Carlo Caione]

On Thu, 2018-07-26 at 15:59 +0200, Philipp Tomsich wrote:
> The calculation in `rockchip_sdram_size` would overflow for 4GB on
> 32bit systems (i.e. when PHYS_64BIT is not defined).
> 
> This makes the internal variables and the signature prototype use a
> u64 to ensure that we can represent the 33rd bit (as in '4GB').
> 

Hi Philipp,
just to let you know that this is still not working on the veyron jerry
chromebook with 4GB I have here (RK3288). The boot stops at:

U-Boot SPL 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)
Trying to boot from SPI

U-Boot 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)

Model: Google Jerry
DRAM:  0 Bytes

I'm still investigating why but for sure there are several points to
fix to have a proper debug, see [0].

Also I was wondering if we should also fix get_effective_memsize() and
gd->bd->bi_dram[].size

[0]https://gist.github.com/carlocaione/b93cfd9ee71e07fcf68d5c02256ff0fa

Cheers,

-- 
Carlo Caione <carlo@endlessm.com>

[U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems

[Dr. Philipp Tomsich]

> On 26 Jul 2018, at 22:05, Carlo Caione <carlo@endlessm.com> wrote:
> 
> On Thu, 2018-07-26 at 15:59 +0200, Philipp Tomsich wrote:
>> The calculation in `rockchip_sdram_size` would overflow for 4GB on
>> 32bit systems (i.e. when PHYS_64BIT is not defined).
>> 
>> This makes the internal variables and the signature prototype use a
>> u64 to ensure that we can represent the 33rd bit (as in '4GB').
>> 
> 
> Hi Philipp,
> just to let you know that this is still not working on the veyron jerry
> chromebook with 4GB I have here (RK3288). The boot stops at:
> 
> U-Boot SPL 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)
> Trying to boot from SPI
> 
> U-Boot 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)
> 
> Model: Google Jerry
> DRAM:  0 Bytes
> 
> I'm still investigating why but for sure there are several points to
> fix to have a proper debug, see [0].
> 
> Also I was wondering if we should also fix get_effective_memsize() and
> gd->bd->bi_dram[].size

Yes, we should. I missed that one…
I only have RK3368 and RK3399 targets to test here, so your testing is very
much appreciated.

> 
> [0]https://gist.github.com/carlocaione/b93cfd9ee71e07fcf68d5c02256ff0fa
> 
> Cheers,
> 
> --
> Carlo Caione <carlo@endlessm.com>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 529 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180726/5fbb638b/attachment.sig>

[U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems

[Dr. Philipp Tomsich]

> On 26 Jul 2018, at 22:05, Carlo Caione <carlo@endlessm.com> wrote:
> 
> On Thu, 2018-07-26 at 15:59 +0200, Philipp Tomsich wrote:
>> The calculation in `rockchip_sdram_size` would overflow for 4GB on
>> 32bit systems (i.e. when PHYS_64BIT is not defined).
>> 
>> This makes the internal variables and the signature prototype use a
>> u64 to ensure that we can represent the 33rd bit (as in '4GB').
>> 
> 
> Hi Philipp,
> just to let you know that this is still not working on the veyron jerry
> chromebook with 4GB I have here (RK3288). The boot stops at:
> 
> U-Boot SPL 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)
> Trying to boot from SPI
> 
> U-Boot 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)
> 
> Model: Google Jerry
> DRAM:  0 Bytes
> 
> I'm still investigating why but for sure there are several points to
> fix to have a proper debug, see [0].
> 
> Also I was wondering if we should also fix get_effective_memsize() and
> gd->bd->bi_dram[].size
> 
> [0]https://gist.github.com/carlocaione/b93cfd9ee71e07fcf68d5c02256ff0fa

Thanks for identifying these two… I’ll pick them up and include in the next
series.

> 
> Cheers,
> 
> -- 
> Carlo Caione <carlo@endlessm.com>

[U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems

[Dr. Philipp Tomsich]

> On 26 Jul 2018, at 22:05, Carlo Caione <carlo@endlessm.com> wrote:
> 
> On Thu, 2018-07-26 at 15:59 +0200, Philipp Tomsich wrote:
>> The calculation in `rockchip_sdram_size` would overflow for 4GB on
>> 32bit systems (i.e. when PHYS_64BIT is not defined).
>> 
>> This makes the internal variables and the signature prototype use a
>> u64 to ensure that we can represent the 33rd bit (as in '4GB').
>> 
> 
> Hi Philipp,
> just to let you know that this is still not working on the veyron jerry
> chromebook with 4GB I have here (RK3288). The boot stops at:
> 
> U-Boot SPL 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)
> Trying to boot from SPI
> 
> U-Boot 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)
> 
> Model: Google Jerry
> DRAM:  0 Bytes
> 
> I'm still investigating why but for sure there are several points to
> fix to have a proper debug, see [0].

I reread ‘rockchip_sdram_size’ and it looks as if I forgot to mark the value
shifted to create chipsize_mb as ULL.  When looking at this code I had an
uneasy feeling that this might run into the C type rules (i.e. 1 will be a 32bit
integer and shifting it by 32 would result in 0), but I didn’t think that this
would ever be the case and that any 4GB DRAM setup would be made
up of multiple channels or of multiple ranks.

> 
> Also I was wondering if we should also fix get_effective_memsize() and
> gd->bd->bi_dram[].size
> 
> [0]https://gist.github.com/carlocaione/b93cfd9ee71e07fcf68d5c02256ff0fa
> 
> Cheers,
> 
> -- 
> Carlo Caione <carlo@endlessm.com>

[U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems

[Carlo Caione]

On Fri, 2018-07-27 at 00:54 +0200, Dr. Philipp Tomsich wrote:
> > On 26 Jul 2018, at 22:05, Carlo Caione <carlo@endlessm.com> wrote:
> > 
> > On Thu, 2018-07-26 at 15:59 +0200, Philipp Tomsich wrote:
> > > The calculation in `rockchip_sdram_size` would overflow for 4GB
> > > on
> > > 32bit systems (i.e. when PHYS_64BIT is not defined).
> > > 
> > > This makes the internal variables and the signature prototype use
> > > a
> > > u64 to ensure that we can represent the 33rd bit (as in '4GB').
> > > 
> > 
> > Hi Philipp,
> > just to let you know that this is still not working on the veyron
> > jerry
> > chromebook with 4GB I have here (RK3288). The boot stops at:
> > 
> > U-Boot SPL 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)
> > Trying to boot from SPI
> > 
> > U-Boot 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)
> > 
> > Model: Google Jerry
> > DRAM:  0 Bytes
> > 
> > I'm still investigating why but for sure there are several points
> > to
> > fix to have a proper debug, see [0].
> 
> I reread ‘rockchip_sdram_size’ and it looks as if I forgot to mark
> the value
> shifted to create chipsize_mb as ULL.  When looking at this code I
> had an
> uneasy feeling that this might run into the C type rules (i.e. 1 will
> be a 32bit
> integer and shifting it by 32 would result in 0), but I didn’t think
> that this
> would ever be the case and that any 4GB DRAM setup would be made
> up of multiple channels or of multiple ranks.

It doesn't hurt but rockchip_sdram_size() is returning the correct
value of 0x100000000 in my tests.

-- 
Carlo Caione <carlo.caione@gmail.com>

[U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems

[Dr. Philipp Tomsich]

> On 27 Jul 2018, at 09:50, Carlo Caione <carlo.caione@gmail.com> wrote:
> 
> On Fri, 2018-07-27 at 00:54 +0200, Dr. Philipp Tomsich wrote:
>>> On 26 Jul 2018, at 22:05, Carlo Caione <carlo@endlessm.com> wrote:
>>> 
>>> On Thu, 2018-07-26 at 15:59 +0200, Philipp Tomsich wrote:
>>>> The calculation in `rockchip_sdram_size` would overflow for 4GB
>>>> on
>>>> 32bit systems (i.e. when PHYS_64BIT is not defined).
>>>> 
>>>> This makes the internal variables and the signature prototype use
>>>> a
>>>> u64 to ensure that we can represent the 33rd bit (as in '4GB').
>>>> 
>>> 
>>> Hi Philipp,
>>> just to let you know that this is still not working on the veyron
>>> jerry
>>> chromebook with 4GB I have here (RK3288). The boot stops at:
>>> 
>>> U-Boot SPL 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)
>>> Trying to boot from SPI
>>> 
>>> U-Boot 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)
>>> 
>>> Model: Google Jerry
>>> DRAM:  0 Bytes
>>> 
>>> I'm still investigating why but for sure there are several points
>>> to
>>> fix to have a proper debug, see [0].
>> 
>> I reread ‘rockchip_sdram_size’ and it looks as if I forgot to mark
>> the value
>> shifted to create chipsize_mb as ULL.  When looking at this code I
>> had an
>> uneasy feeling that this might run into the C type rules (i.e. 1 will
>> be a 32bit
>> integer and shifting it by 32 would result in 0), but I didn’t think
>> that this
>> would ever be the case and that any 4GB DRAM setup would be made
>> up of multiple channels or of multiple ranks.
> 
> It doesn't hurt but rockchip_sdram_size() is returning the correct
> value of 0x100000000 in my tests.

The 'gd->bd->bi_dram[i].size’ path will also be involved and truncate this
later on, but I am not convinced that it’s worth fixing the dram banks info
for the general case.

Generally, typing for these bi_dram structures seems a mess: they are
often cast to 32bit and sometimes to 64bit in common code (i.e. fdtdec.c
uses unsigned long long).
I hope I can avoid touching this code.

Btw, here’s a gem from board_f.c (these two lines are after each other):
>         gd->ram_top = gd->ram_base + get_effective_memsize();
>         gd->ram_top = board_get_usable_ram_top(gd->mon_len);

As the first line is clearly deal (except the fact that the compiler can’t tell
that get_effective_memsize() should be side-effect free), we can drop it.
I’ll send a separate patch for this to be picked up by Tom…

—Phil.

[U-Boot] [PATCH 3/9] common: include <inttypes.h> always

[Masahiro Yamada]

2018-07-26 22:59 GMT+09:00 Philipp Tomsich
<philipp.tomsich@theobroma-systems.com>:
> With the ram-size variable changed to u64, we'll need appropriate
> macros for printing u64 values correctly either in 32bit builds
> (i.e. ILP32 models) or in 64bit builds (i.e. LP64 models).  Best to
> make the PRIx64 macro available everywhere.
>
> This include inttypes.h from common.h to make the various macros for
> formatted printing available to everyone.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---


NACK.


PRIx64 is evil crap. I would make the code super ugly.
Do not use it.


The right thing to do is use the same typedefs
for all architectures.

typedef unsigned char       u8;
typedef unsigned short      u16;
typedef unsigned int        u32;
typedef unsigned long long  u64;

This works for both ILP32 and LP64.


Use '%llx' for printing u64 variables _always_.



This is what Linux exactly does.



In fact, Linux defined fixed-width types differently
for different architectures in old days.


After long time effort, Linux unified
the fixed-width types for the kernel space.

https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/int-ll64.h



See Linux commit:

commit 0c79a8e29b5fcbcbfd611daf9d500cfad8370fcf
Author: Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Thu Jan 23 15:53:43 2014 -0800

    asm/types.h: Remove include/asm-generic/int-l64.h





And, I fixed ARM Trusted Firmware in the same way:

https://github.com/ARM-software/arm-trusted-firmware/commit/0a2d5b43c81ed6132761023bf43755f13122ddf0





U-Boot is still doing wrong,
and core developers in U-Boot do not understand this, unfortunately.









>  include/common.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/common.h b/include/common.h
> index 940161f..9de9145 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -30,6 +30,7 @@ typedef volatile unsigned char        vu_char;
>  #include <linux/stringify.h>
>  #include <asm/ptrace.h>
>  #include <stdarg.h>
> +#include <inttypes.h>
>  #include <stdio.h>
>  #include <linux/kernel.h>
>
> --
> 2.1.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 8/9] board: cm_fx6: use PRIx64 macros for printing ram size

[Masahiro Yamada]

2018-07-26 22:59 GMT+09:00 Philipp Tomsich
<philipp.tomsich@theobroma-systems.com>:
> With the recent changes of the underlying types for the ram size, we
> need to adjust the formatting.  Use PRIx64 to print the (now) u64
> field.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---


As I commented in 3/9,
using PRIx64 is wrong.





>  board/compulab/cm_fx6/cm_fx6.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
> index c114cdc..b163abf 100644
> --- a/board/compulab/cm_fx6/cm_fx6.c
> +++ b/board/compulab/cm_fx6/cm_fx6.c
> @@ -704,7 +704,8 @@ int dram_init(void)
>                 gd->ram_size -= 0x100000;
>                 break;
>         default:
> -               printf("ERROR: Unsupported DRAM size 0x%lx\n", gd->ram_size);
> +               printf("ERROR: Unsupported DRAM size 0x%" PRIx64 "\n",
> +                      gd->ram_size);
>                 return -1;
>         }
>
> --
> 2.1.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 3/9] common: include <inttypes.h> always

[Patrick DELAUNAY]

Hi Philipp

> -----Original Message-----
> From: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Sent: jeudi 26 juillet 2018 16:00
> 
> With the ram-size variable changed to u64, we'll need appropriate macros for
> printing u64 values correctly either in 32bit builds (i.e. ILP32 models) or in 64bit
> builds (i.e. LP64 models).  Best to make the PRIx64 macro available everywhere.
> 
> This include inttypes.h from common.h to make the various macros for
> formatted printing available to everyone.
> 
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
> 
>  include/common.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/common.h b/include/common.h index 940161f..9de9145
> 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -30,6 +30,7 @@ typedef volatile unsigned char	vu_char;
>  #include <linux/stringify.h>
>  #include <asm/ptrace.h>
>  #include <stdarg.h>
> +#include <inttypes.h>
>  #include <stdio.h>
>  #include <linux/kernel.h>

I think the include should be moved after the line 40
and __STDC_FORMAT_MACROS definition  :

-/* Bring in printf format macros if inttypes.h is included */
+/* Bring in printf format macros defined in inttypes.h */
#define __STDC_FORMAT_MACROS
+#include <inttypes.h>

Regards
Patrick

[U-Boot] [PATCH 6/9] ram: stm32mp1: use PRIx64 macros for printing ram size

[Patrick DELAUNAY]

Hi Philipp

> -----Original Message-----
> From: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Sent: jeudi 26 juillet 2018 16:00
> 
> With the recent changes of the underlying types for the ram size, we need to
> adjust the formatting.  Use PRIx64 to print the (now) u64 field.
> 
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
> 
>  arch/arm/mach-stm32mp/dram_init.c   | 2 +-
>  drivers/ram/stm32mp1/stm32mp1_ram.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-stm32mp/dram_init.c b/arch/arm/mach-
> stm32mp/dram_init.c
> index 7688b3e..e4c6302 100644
> --- a/arch/arm/mach-stm32mp/dram_init.c
> +++ b/arch/arm/mach-stm32mp/dram_init.c
> @@ -25,7 +25,7 @@ int dram_init(void)
>  		debug("Cannot get RAM size: %d\n", ret);
>  		return ret;
>  	}
> -	debug("RAM init base=%lx, size=%x\n", ram.base, ram.size);
> +	debug("RAM init base=%lx, size=%" PRIx64 "\n", ram.base, ram.size);
> 
>  	gd->ram_size = ram.size;
> 
> diff --git a/drivers/ram/stm32mp1/stm32mp1_ram.c
> b/drivers/ram/stm32mp1/stm32mp1_ram.c
> index bd497a3..faf78b2 100644
> --- a/drivers/ram/stm32mp1/stm32mp1_ram.c
> +++ b/drivers/ram/stm32mp1/stm32mp1_ram.c
> @@ -130,7 +130,7 @@ static __maybe_unused int stm32mp1_ddr_setup(struct
> udevice *dev)
> 
>  	/* check memory access for all memory */
>  	if (config.info.size != priv->info.size) {
> -		printf("DDR invalid size : 0x%x, expected 0x%x\n",
> +		printf("DDR invalid size : 0x%" PRIx64 ", expected 0x%x\n",
>  		       priv->info.size, config.info.size);
>  		return -EINVAL;
>  	}
> --
> 2.1.4

Tested on STM32MP157c EV1 board with debug activated for RAM driver.
The traces are displayed correctly.

Reviewed-by: Patrick Delaunay <patrick.delaunay@st.com>
Tested-by: Patrick Delaunay <patrick.delaunay@st.com>

Regard
Patrick

[U-Boot] [PATCH 6/9] ram: stm32mp1: use PRIx64 macros for printing ram size

[Simon Glass]

On 26 July 2018 at 07:59, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
>
> With the recent changes of the underlying types for the ram size, we
> need to adjust the formatting.  Use PRIx64 to print the (now) u64
> field.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
>  arch/arm/mach-stm32mp/dram_init.c   | 2 +-
>  drivers/ram/stm32mp1/stm32mp1_ram.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 3/9] common: include <inttypes.h> always

[Simon Glass]

+Tom

Hi Masahiro,

On 30 July 2018 at 19:41, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> 2018-07-26 22:59 GMT+09:00 Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com>:
>> With the ram-size variable changed to u64, we'll need appropriate
>> macros for printing u64 values correctly either in 32bit builds
>> (i.e. ILP32 models) or in 64bit builds (i.e. LP64 models).  Best to
>> make the PRIx64 macro available everywhere.
>>
>> This include inttypes.h from common.h to make the various macros for
>> formatted printing available to everyone.
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>
>
> NACK.
>
>
> PRIx64 is evil crap. I would make the code super ugly.
> Do not use it.
>
>
> The right thing to do is use the same typedefs
> for all architectures.
>
> typedef unsigned char       u8;
> typedef unsigned short      u16;
> typedef unsigned int        u32;
> typedef unsigned long long  u64;
>
> This works for both ILP32 and LP64.
>
>
> Use '%llx' for printing u64 variables _always_.
>
>
>
> This is what Linux exactly does.
>
>
>
> In fact, Linux defined fixed-width types differently
> for different architectures in old days.
>
>
> After long time effort, Linux unified
> the fixed-width types for the kernel space.
>
> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/int-ll64.h
>
>
>
> See Linux commit:
>
> commit 0c79a8e29b5fcbcbfd611daf9d500cfad8370fcf
> Author: Geert Uytterhoeven <geert@linux-m68k.org>
> Date:   Thu Jan 23 15:53:43 2014 -0800
>
>     asm/types.h: Remove include/asm-generic/int-l64.h
>
>
>
>
>
> And, I fixed ARM Trusted Firmware in the same way:
>
> https://github.com/ARM-software/arm-trusted-firmware/commit/0a2d5b43c81ed6132761023bf43755f13122ddf0
>
>
>
>
>
> U-Boot is still doing wrong,
> and core developers in U-Boot do not understand this, unfortunately.

While this works in many cases we do seem to have problems with some
toolchains. Perhaps things are better now as my problems were a a few
years back. Things like size_t with %z caused problems too. I remember
m68k producing warnings when I tried this.

I am certainly interested in converting over to this other approach. I
am also OK with the PRi stuff, since it only affects a relatively
small number of cases.

Regards,
Simon

[U-Boot] [PATCH 1/9] dm: allow 4GB of DRAM on 32bit systems

[Simon Glass]

On 26 July 2018 at 07:59, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> Even on 32bit systems a full 4GB of DRAM may be installed and reported
> by the DRAM controller.  Whether these 4GB are larger available
> depends on the size/configuration of address decoding windows and
> architectural features (e.g. consider a hypothetical architecture that
> uses dedicated instructions to access the 'memory-mapped' peripheral
> IO ranges).  In U-Boot, the available DRAM, as reported by the
> device-model is independent of the accessible DRAM (i.e. adjusted top
> and effective size).
>
> This was first reported against the RK3288, which may have 4GB DRAM
> attached, but will have a small (e.g. 128MB) window not accessible due
> to address decoding limitations.
>
> The solution is to increase the types used for storing the ram_size to
> have at least 33 bits even on 32bit systems... i.e. we need to use a
> u64 for these always (this was previously only the case for
> PHYS_64BIT, which will have unwanted side-effects for 32bit systems
> and would require more intrusive changes).
>
> This commit changes the size-field in 'struct ram' and the ram_size in
> 'gd' to be a u64.
>
> Reported-by: Marty E. Plummer <hanetzer@startmail.com>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
>  include/asm-generic/global_data.h | 2 +-
>  include/ram.h                     | 9 ++++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index 0fd4900..f3d687c 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -55,7 +55,7 @@ typedef struct global_data {
>         unsigned long ram_base;         /* Base address of RAM used by U-Boot */
>         unsigned long ram_top;          /* Top address of RAM used by U-Boot */
>         unsigned long relocaddr;        /* Start address of U-Boot in RAM */
> -       phys_size_t ram_size;           /* RAM size */
> +       u64 ram_size;                   /* RAM size */
>         unsigned long mon_len;          /* monitor len */
>         unsigned long irq_sp;           /* irq stack pointer */
>         unsigned long start_addr_sp;    /* start_addr_stackpointer */
> diff --git a/include/ram.h b/include/ram.h
> index 67e22d7..1fe024f 100644
> --- a/include/ram.h
> +++ b/include/ram.h
> @@ -9,7 +9,14 @@
>
>  struct ram_info {
>         phys_addr_t base;
> -       size_t size;
> +       /*
> +        * We use a 64bit type for the size to correctly handle 32bit
> +        * systems with 4GB of DRAM (which would overflow a 32bit type
> +        * and read back as 0).  For 64bit systems, we assume (for now)

forever :-)

> +        * that they will always have less than 2^65 byte of DRAM
> +        * installed. -- prt

Is the prt your signature? I suggest dropping it.

> +        */
> +       u64 size;
>  };
>
>  struct ram_ops {
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH 4/9] MIPS: use PRIx64 macros for printing ram size

[Simon Glass]

On 26 July 2018 at 07:59, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> With the recent changes of the underlying types for the ram size, we need
> to adjust the formatting.  Use PRIx64 to print the (now) u64 field.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
>  arch/mips/mach-bmips/dram.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 5/9] rockchip: rk3368: change type of ram-size field for a min()-calculation

[Simon Glass]

Hi Phiipp,

On 26 July 2018 at 07:59, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> With the recent changes of the underlying types for the ram size, we need
> to cast the constant used in clamping to u64 instead of size_t.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
>  drivers/ram/rockchip/dmc-rk3368.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/drivers/ram/rockchip/dmc-rk3368.c b/drivers/ram/rockchip/dmc-rk3368.c
> index 8d1b9fa..35d7605 100644
> --- a/drivers/ram/rockchip/dmc-rk3368.c
> +++ b/drivers/ram/rockchip/dmc-rk3368.c
> @@ -960,7 +960,7 @@ static int rk3368_dmc_probe(struct udevice *dev)
>         * is SoC register space (i.e. reserved), and 0xfe000000~0xfeffffff is
>         * inaccessible for some IP controller.
>         */
> -       priv->info.size = min(priv->info.size, (size_t)0xfe000000);
> +       priv->info.size = min(priv->info.size, (u64)0xfe000000);

Why case at all? Is it because min() is a macro?

>
>         return 0;
>  }
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH 7/9] board: keymile: add explicit cast to truncate the 64bit ram size field

[Simon Glass]

On 26 July 2018 at 07:59, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> With the change to a 64bit ram size field for 32bit architectures, we need
> to add an explicit cast to truncate when converting to a pointer.
> This casts to 'void*' through 'uintptr_t'.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
>  board/keymile/km_arm/km_arm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 8/9] board: cm_fx6: use PRIx64 macros for printing ram size

[Simon Glass]

On 30 July 2018 at 19:42, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> 2018-07-26 22:59 GMT+09:00 Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com>:
>> With the recent changes of the underlying types for the ram size, we
>> need to adjust the formatting.  Use PRIx64 to print the (now) u64
>> field.
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>
>
> As I commented in 3/9,
> using PRIx64 is wrong.
>

I'm happy enough if we can more to using fixed types (long int =
32-bit, long long = 64-bit). But is anyone actually working on this?

For now this is standard C and seems good enough to me.

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 1/9] dm: allow 4GB of DRAM on 32bit systems

[Dr. Philipp Tomsich]

> On 2 Aug 2018, at 22:36, Simon Glass <sjg@chromium.org> wrote:
> 
> On 26 July 2018 at 07:59, Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> wrote:
>> Even on 32bit systems a full 4GB of DRAM may be installed and reported
>> by the DRAM controller.  Whether these 4GB are larger available
>> depends on the size/configuration of address decoding windows and
>> architectural features (e.g. consider a hypothetical architecture that
>> uses dedicated instructions to access the 'memory-mapped' peripheral
>> IO ranges).  In U-Boot, the available DRAM, as reported by the
>> device-model is independent of the accessible DRAM (i.e. adjusted top
>> and effective size).
>> 
>> This was first reported against the RK3288, which may have 4GB DRAM
>> attached, but will have a small (e.g. 128MB) window not accessible due
>> to address decoding limitations.
>> 
>> The solution is to increase the types used for storing the ram_size to
>> have at least 33 bits even on 32bit systems... i.e. we need to use a
>> u64 for these always (this was previously only the case for
>> PHYS_64BIT, which will have unwanted side-effects for 32bit systems
>> and would require more intrusive changes).
>> 
>> This commit changes the size-field in 'struct ram' and the ram_size in
>> 'gd' to be a u64.
>> 
>> Reported-by: Marty E. Plummer <hanetzer@startmail.com>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>> 
>> include/asm-generic/global_data.h | 2 +-
>> include/ram.h                     | 9 ++++++++-
>> 2 files changed, 9 insertions(+), 2 deletions(-)
>> 
> 
> Reviewed-by: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>
> 
>> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
>> index 0fd4900..f3d687c 100644
>> --- a/include/asm-generic/global_data.h
>> +++ b/include/asm-generic/global_data.h
>> @@ -55,7 +55,7 @@ typedef struct global_data {
>>        unsigned long ram_base;         /* Base address of RAM used by U-Boot */
>>        unsigned long ram_top;          /* Top address of RAM used by U-Boot */
>>        unsigned long relocaddr;        /* Start address of U-Boot in RAM */
>> -       phys_size_t ram_size;           /* RAM size */
>> +       u64 ram_size;                   /* RAM size */
>>        unsigned long mon_len;          /* monitor len */
>>        unsigned long irq_sp;           /* irq stack pointer */
>>        unsigned long start_addr_sp;    /* start_addr_stackpointer */
>> diff --git a/include/ram.h b/include/ram.h
>> index 67e22d7..1fe024f 100644
>> --- a/include/ram.h
>> +++ b/include/ram.h
>> @@ -9,7 +9,14 @@
>> 
>> struct ram_info {
>>        phys_addr_t base;
>> -       size_t size;
>> +       /*
>> +        * We use a 64bit type for the size to correctly handle 32bit
>> +        * systems with 4GB of DRAM (which would overflow a 32bit type
>> +        * and read back as 0).  For 64bit systems, we assume (for now)
> 
> forever :-)
> 
>> +        * that they will always have less than 2^65 byte of DRAM
>> +        * installed. -- prt
> 
> Is the prt your signature? I suggest dropping it.

In fact it is. But I’ll need to rewrite the entire comment anyway for the next
version of this series as there’s even more functions and places that the
memory size is stored in...

> 
>> +        */
>> +       u64 size;
>> };
>> 
>> struct ram_ops {
>> --
>> 2.1.4
>> 
> 
> Regards,
> Simon

[U-Boot] [PATCH 3/9] common: include <inttypes.h> always

[Tom Rini]

On Thu, Aug 02, 2018 at 10:56:11AM -0600, Simon Glass wrote:
> +Tom
> 
> Hi Masahiro,
> 
> On 30 July 2018 at 19:41, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> > 2018-07-26 22:59 GMT+09:00 Philipp Tomsich
> > <philipp.tomsich@theobroma-systems.com>:
> >> With the ram-size variable changed to u64, we'll need appropriate
> >> macros for printing u64 values correctly either in 32bit builds
> >> (i.e. ILP32 models) or in 64bit builds (i.e. LP64 models).  Best to
> >> make the PRIx64 macro available everywhere.
> >>
> >> This include inttypes.h from common.h to make the various macros for
> >> formatted printing available to everyone.
> >>
> >> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> >> ---
> >
> >
> > NACK.
> >
> >
> > PRIx64 is evil crap. I would make the code super ugly.
> > Do not use it.
> >
> >
> > The right thing to do is use the same typedefs
> > for all architectures.
> >
> > typedef unsigned char       u8;
> > typedef unsigned short      u16;
> > typedef unsigned int        u32;
> > typedef unsigned long long  u64;
> >
> > This works for both ILP32 and LP64.
> >
> >
> > Use '%llx' for printing u64 variables _always_.
> >
> >
> >
> > This is what Linux exactly does.
> >
> >
> >
> > In fact, Linux defined fixed-width types differently
> > for different architectures in old days.
> >
> >
> > After long time effort, Linux unified
> > the fixed-width types for the kernel space.
> >
> > https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/int-ll64.h
> >
> >
> >
> > See Linux commit:
> >
> > commit 0c79a8e29b5fcbcbfd611daf9d500cfad8370fcf
> > Author: Geert Uytterhoeven <geert@linux-m68k.org>
> > Date:   Thu Jan 23 15:53:43 2014 -0800
> >
> >     asm/types.h: Remove include/asm-generic/int-l64.h
> >
> >
> >
> >
> >
> > And, I fixed ARM Trusted Firmware in the same way:
> >
> > https://github.com/ARM-software/arm-trusted-firmware/commit/0a2d5b43c81ed6132761023bf43755f13122ddf0
> >
> >
> >
> >
> >
> > U-Boot is still doing wrong,
> > and core developers in U-Boot do not understand this, unfortunately.
> 
> While this works in many cases we do seem to have problems with some
> toolchains. Perhaps things are better now as my problems were a a few
> years back. Things like size_t with %z caused problems too. I remember
> m68k producing warnings when I tried this.
> 
> I am certainly interested in converting over to this other approach. I
> am also OK with the PRi stuff, since it only affects a relatively
> small number of cases.

It would certainly be worth giving things another try with current
compilers.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180803/467aba57/attachment.sig>

[U-Boot] [PATCH 3/9] common: include <inttypes.h> always

[Masahiro Yamada]

2018-08-04 0:01 GMT+09:00 Tom Rini <trini@konsulko.com>:
> On Thu, Aug 02, 2018 at 10:56:11AM -0600, Simon Glass wrote:
>> +Tom
>>
>> Hi Masahiro,
>>
>> On 30 July 2018 at 19:41, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> > 2018-07-26 22:59 GMT+09:00 Philipp Tomsich
>> > <philipp.tomsich@theobroma-systems.com>:
>> >> With the ram-size variable changed to u64, we'll need appropriate
>> >> macros for printing u64 values correctly either in 32bit builds
>> >> (i.e. ILP32 models) or in 64bit builds (i.e. LP64 models).  Best to
>> >> make the PRIx64 macro available everywhere.
>> >>
>> >> This include inttypes.h from common.h to make the various macros for
>> >> formatted printing available to everyone.
>> >>
>> >> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> >> ---
>> >
>> >
>> > NACK.
>> >
>> >
>> > PRIx64 is evil crap. I would make the code super ugly.
>> > Do not use it.
>> >
>> >
>> > The right thing to do is use the same typedefs
>> > for all architectures.
>> >
>> > typedef unsigned char       u8;
>> > typedef unsigned short      u16;
>> > typedef unsigned int        u32;
>> > typedef unsigned long long  u64;
>> >
>> > This works for both ILP32 and LP64.
>> >
>> >
>> > Use '%llx' for printing u64 variables _always_.
>> >
>> >
>> >
>> > This is what Linux exactly does.
>> >
>> >
>> >
>> > In fact, Linux defined fixed-width types differently
>> > for different architectures in old days.
>> >
>> >
>> > After long time effort, Linux unified
>> > the fixed-width types for the kernel space.
>> >
>> > https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/int-ll64.h
>> >
>> >
>> >
>> > See Linux commit:
>> >
>> > commit 0c79a8e29b5fcbcbfd611daf9d500cfad8370fcf
>> > Author: Geert Uytterhoeven <geert@linux-m68k.org>
>> > Date:   Thu Jan 23 15:53:43 2014 -0800
>> >
>> >     asm/types.h: Remove include/asm-generic/int-l64.h
>> >
>> >
>> >
>> >
>> >
>> > And, I fixed ARM Trusted Firmware in the same way:
>> >
>> > https://github.com/ARM-software/arm-trusted-firmware/commit/0a2d5b43c81ed6132761023bf43755f13122ddf0
>> >
>> >
>> >
>> >
>> >
>> > U-Boot is still doing wrong,
>> > and core developers in U-Boot do not understand this, unfortunately.
>>
>> While this works in many cases we do seem to have problems with some
>> toolchains. Perhaps things are better now as my problems were a a few
>> years back.


Please pin-point the pre-built toolchains with which
you had problems.


In other words, please show a compiler that has:

  sizeof(int) != 4

        or

  sizeof(long long) != 8




>> Things like size_t with %z caused problems too. I remember
>> m68k producing warnings when I tried this.


This is true, but
the typedefs of uint{8,16,32,64}_t
and the typedef of size_t are
_different_ problems.

Please do not mix them up to exaggerate things.


Most 32 bit architectures use "unsigned int" size_t,
and all 64 bit architectures use "unsigned long" size_t
as stated in
https://github.com/torvalds/linux/blob/v4.17/include/uapi/asm-generic/posix_types.h#L63


Linux kernel hard-codes size_t and ssize_t, thus
people are supposed to use a compiler with proper size_t define.

Such compilers are provided in
https://mirrors.edge.kernel.org/pub/tools/crosstool/



If you are not happy about hard-coding size_t,
you can do like this:


typedef __SIZE_TYPE__ size_t;

typedef __typeof(__builtin_choose_expr(__builtin_types_compatible_p(size_t,
unsigned long), (long)0, (int)0)) ssize_t;




>> I am certainly interested in converting over to this other approach. I
>> am also OK with the PRi stuff, since it only affects a relatively
>> small number of cases.

Whether the code is right or not
should not be judged by the number of use cases.




> It would certainly be worth giving things another try with current
> compilers.
>
> --
> Tom
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 4/9] MIPS: use PRIx64 macros for printing ram size

[Daniel Schwierzeck]

2018-07-26 15:59 GMT+02:00 Philipp Tomsich
<philipp.tomsich@theobroma-systems.com>:
> With the recent changes of the underlying types for the ram size, we need
> to adjust the formatting.  Use PRIx64 to print the (now) u64 field.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
>  arch/mips/mach-bmips/dram.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>

-- 
- Daniel

[U-Boot] [PATCH 0/9] Support 4GB of memory on 32bit systems

[Tom Rini]

On Thu, Jul 26, 2018 at 03:59:42PM +0200, Philipp Tomsich wrote:

> Even on 32bit systems a full 4GB of DRAM may be installed and reported
> by the DRAM controller.  Whether these 4GB are larger available
> depends on the size/configuration of address decoding windows and
> architectural features.
> 
> This increases the fields holding the RAM size to have at least 33bits
> (i.e. we use a u64) and fixes the fallout from the change: some casts,
> the usage of min() and a few printf formats have to be adjusted.
> 
> 
> Philipp Tomsich (9):
>   dm: allow 4GB of DRAM on 32bit systems
>   rockchip: support 4GB DRAM on 32bit systems
>   common: include <inttypes.h> always
>   MIPS: use PRIx64 macros for printing ram size
>   rockchip: rk3368: change type of ram-size field for a
>     min()-calculation
>   ram: stm32mp1: use PRIx64 macros for printing ram size
>   board: keymile: add explicit cast to truncate the 64bit ram size field
>   board: cm_fx6: use PRIx64 macros for printing ram size
>   mpc85xx: add casts for ram size in min() calculation.
> 
>  arch/arm/include/asm/arch-rockchip/sdram_common.h | 2 +-
>  arch/arm/mach-rockchip/sdram_common.c             | 8 ++++----
>  arch/arm/mach-stm32mp/dram_init.c                 | 2 +-
>  arch/mips/mach-bmips/dram.c                       | 2 +-
>  arch/powerpc/cpu/mpc85xx/cpu.c                    | 6 ++++--
>  board/compulab/cm_fx6/cm_fx6.c                    | 3 ++-
>  board/keymile/km_arm/km_arm.c                     | 6 ++++--
>  drivers/ram/rockchip/dmc-rk3368.c                 | 2 +-
>  drivers/ram/stm32mp1/stm32mp1_ram.c               | 2 +-
>  include/asm-generic/global_data.h                 | 2 +-
>  include/common.h                                  | 1 +
>  include/ram.h                                     | 9 ++++++++-
>  12 files changed, 29 insertions(+), 16 deletions(-)

Please rework this on top of
https://patchwork.ozlabs.org/user/todo/uboot/?series=59452 thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180810/ab559ac5/attachment.sig>

[U-Boot] [PATCH 1/9] dm: allow 4GB of DRAM on 32bit systems

[Alexander Graf]

On 02.08.18 23:31, Dr. Philipp Tomsich wrote:
> 
>> On 2 Aug 2018, at 22:36, Simon Glass <sjg@chromium.org> wrote:
>>
>> On 26 July 2018 at 07:59, Philipp Tomsich
>> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> wrote:
>>> Even on 32bit systems a full 4GB of DRAM may be installed and reported
>>> by the DRAM controller.  Whether these 4GB are larger available
>>> depends on the size/configuration of address decoding windows and
>>> architectural features (e.g. consider a hypothetical architecture that
>>> uses dedicated instructions to access the 'memory-mapped' peripheral
>>> IO ranges).  In U-Boot, the available DRAM, as reported by the
>>> device-model is independent of the accessible DRAM (i.e. adjusted top
>>> and effective size).
>>>
>>> This was first reported against the RK3288, which may have 4GB DRAM
>>> attached, but will have a small (e.g. 128MB) window not accessible due
>>> to address decoding limitations.
>>>
>>> The solution is to increase the types used for storing the ram_size to
>>> have at least 33 bits even on 32bit systems... i.e. we need to use a
>>> u64 for these always (this was previously only the case for
>>> PHYS_64BIT, which will have unwanted side-effects for 32bit systems
>>> and would require more intrusive changes).
>>>
>>> This commit changes the size-field in 'struct ram' and the ram_size in
>>> 'gd' to be a u64.
>>>
>>> Reported-by: Marty E. Plummer <hanetzer@startmail.com>
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>> ---
>>>
>>> include/asm-generic/global_data.h | 2 +-
>>> include/ram.h                     | 9 ++++++++-
>>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>>
>>
>> Reviewed-by: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>
>>
>>> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
>>> index 0fd4900..f3d687c 100644
>>> --- a/include/asm-generic/global_data.h
>>> +++ b/include/asm-generic/global_data.h
>>> @@ -55,7 +55,7 @@ typedef struct global_data {
>>>        unsigned long ram_base;         /* Base address of RAM used by U-Boot */
>>>        unsigned long ram_top;          /* Top address of RAM used by U-Boot */
>>>        unsigned long relocaddr;        /* Start address of U-Boot in RAM */
>>> -       phys_size_t ram_size;           /* RAM size */
>>> +       u64 ram_size;                   /* RAM size */
>>>        unsigned long mon_len;          /* monitor len */
>>>        unsigned long irq_sp;           /* irq stack pointer */
>>>        unsigned long start_addr_sp;    /* start_addr_stackpointer */
>>> diff --git a/include/ram.h b/include/ram.h
>>> index 67e22d7..1fe024f 100644
>>> --- a/include/ram.h
>>> +++ b/include/ram.h
>>> @@ -9,7 +9,14 @@
>>>
>>> struct ram_info {
>>>        phys_addr_t base;
>>> -       size_t size;
>>> +       /*
>>> +        * We use a 64bit type for the size to correctly handle 32bit
>>> +        * systems with 4GB of DRAM (which would overflow a 32bit type
>>> +        * and read back as 0).  For 64bit systems, we assume (for now)
>>
>> forever :-)
>>
>>> +        * that they will always have less than 2^65 byte of DRAM
>>> +        * installed. -- prt
>>
>> Is the prt your signature? I suggest dropping it.
> 
> In fact it is. But I’ll need to rewrite the entire comment anyway for the next
> version of this series as there’s even more functions and places that the
> memory size is stored in...
> 
>>
>>> +        */
>>> +       u64 size;

With LPAE available in all modern ARM cores, shouldn't phys_addr_t just
be u64? And then we'd probably want to use that throughout the code, right?


Alex

[U-Boot] [PATCH 1/9] dm: allow 4GB of DRAM on 32bit systems

[Vagrant Cascadian]

On 2018-09-02, Alexander Graf <agraf@suse.de> wrote:
> On 02.08.18 23:31, Dr. Philipp Tomsich wrote:
>> 
>>> On 2 Aug 2018, at 22:36, Simon Glass <sjg@chromium.org> wrote:
>>>
>>> On 26 July 2018 at 07:59, Philipp Tomsich
>>> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> wrote:
>>>> Even on 32bit systems a full 4GB of DRAM may be installed and reported
>>>> by the DRAM controller.  Whether these 4GB are larger available
>>>> depends on the size/configuration of address decoding windows and
>>>> architectural features (e.g. consider a hypothetical architecture that
>>>> uses dedicated instructions to access the 'memory-mapped' peripheral
>>>> IO ranges).  In U-Boot, the available DRAM, as reported by the
>>>> device-model is independent of the accessible DRAM (i.e. adjusted top
>>>> and effective size).
>>>>
>>>> This was first reported against the RK3288, which may have 4GB DRAM
>>>> attached, but will have a small (e.g. 128MB) window not accessible due
>>>> to address decoding limitations.
>>>>
>>>> The solution is to increase the types used for storing the ram_size to
>>>> have at least 33 bits even on 32bit systems... i.e. we need to use a
>>>> u64 for these always (this was previously only the case for
>>>> PHYS_64BIT, which will have unwanted side-effects for 32bit systems
>>>> and would require more intrusive changes).
>>>>
>>>> This commit changes the size-field in 'struct ram' and the ram_size in
>>>> 'gd' to be a u64.
>>>>
>>>> Reported-by: Marty E. Plummer <hanetzer@startmail.com>
>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>> ---
>>>>
>>>> include/asm-generic/global_data.h | 2 +-
>>>> include/ram.h                     | 9 ++++++++-
>>>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>
>>> Reviewed-by: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>
>>>
>>>> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
>>>> index 0fd4900..f3d687c 100644
>>>> --- a/include/asm-generic/global_data.h
>>>> +++ b/include/asm-generic/global_data.h
>>>> @@ -55,7 +55,7 @@ typedef struct global_data {
>>>>        unsigned long ram_base;         /* Base address of RAM used by U-Boot */
>>>>        unsigned long ram_top;          /* Top address of RAM used by U-Boot */
>>>>        unsigned long relocaddr;        /* Start address of U-Boot in RAM */
>>>> -       phys_size_t ram_size;           /* RAM size */
>>>> +       u64 ram_size;                   /* RAM size */
>>>>        unsigned long mon_len;          /* monitor len */
>>>>        unsigned long irq_sp;           /* irq stack pointer */
>>>>        unsigned long start_addr_sp;    /* start_addr_stackpointer */
>>>> diff --git a/include/ram.h b/include/ram.h
>>>> index 67e22d7..1fe024f 100644
>>>> --- a/include/ram.h
>>>> +++ b/include/ram.h
>>>> @@ -9,7 +9,14 @@
>>>>
>>>> struct ram_info {
>>>>        phys_addr_t base;
>>>> -       size_t size;
>>>> +       /*
>>>> +        * We use a 64bit type for the size to correctly handle 32bit
>>>> +        * systems with 4GB of DRAM (which would overflow a 32bit type
>>>> +        * and read back as 0).  For 64bit systems, we assume (for now)
>>>
>>> forever :-)
>>>
>>>> +        * that they will always have less than 2^65 byte of DRAM
>>>> +        * installed. -- prt
>>>
>>> Is the prt your signature? I suggest dropping it.
>> 
>> In fact it is. But I’ll need to rewrite the entire comment anyway for the next
>> version of this series as there’s even more functions and places that the
>> memory size is stored in...
>> 
>>>
>>>> +        */
>>>> +       u64 size;
>
> With LPAE available in all modern ARM cores, shouldn't phys_addr_t just
> be u64? And then we'd probably want to use that throughout the code, right?

Quite a few currently supported boards do not support LPAE, e.g. imx6.


live well,
  vagrant
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180902/a1a2ae0a/attachment.sig>

[U-Boot] [PATCH 1/9] dm: allow 4GB of DRAM on 32bit systems

[Alexander Graf]

> Am 02.09.2018 um 18:04 schrieb Vagrant Cascadian <vagrant@debian.org>:
> 
>> On 2018-09-02, Alexander Graf <agraf@suse.de> wrote:
>>> On 02.08.18 23:31, Dr. Philipp Tomsich wrote:
>>> 
>>>> On 2 Aug 2018, at 22:36, Simon Glass <sjg@chromium.org> wrote:
>>>> 
>>>> On 26 July 2018 at 07:59, Philipp Tomsich
>>>> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> wrote:
>>>>> Even on 32bit systems a full 4GB of DRAM may be installed and reported
>>>>> by the DRAM controller.  Whether these 4GB are larger available
>>>>> depends on the size/configuration of address decoding windows and
>>>>> architectural features (e.g. consider a hypothetical architecture that
>>>>> uses dedicated instructions to access the 'memory-mapped' peripheral
>>>>> IO ranges).  In U-Boot, the available DRAM, as reported by the
>>>>> device-model is independent of the accessible DRAM (i.e. adjusted top
>>>>> and effective size).
>>>>> 
>>>>> This was first reported against the RK3288, which may have 4GB DRAM
>>>>> attached, but will have a small (e.g. 128MB) window not accessible due
>>>>> to address decoding limitations.
>>>>> 
>>>>> The solution is to increase the types used for storing the ram_size to
>>>>> have at least 33 bits even on 32bit systems... i.e. we need to use a
>>>>> u64 for these always (this was previously only the case for
>>>>> PHYS_64BIT, which will have unwanted side-effects for 32bit systems
>>>>> and would require more intrusive changes).
>>>>> 
>>>>> This commit changes the size-field in 'struct ram' and the ram_size in
>>>>> 'gd' to be a u64.
>>>>> 
>>>>> Reported-by: Marty E. Plummer <hanetzer@startmail.com>
>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>> ---
>>>>> 
>>>>> include/asm-generic/global_data.h | 2 +-
>>>>> include/ram.h                     | 9 ++++++++-
>>>>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>>>> 
>>>> 
>>>> Reviewed-by: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>
>>>> 
>>>>> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
>>>>> index 0fd4900..f3d687c 100644
>>>>> --- a/include/asm-generic/global_data.h
>>>>> +++ b/include/asm-generic/global_data.h
>>>>> @@ -55,7 +55,7 @@ typedef struct global_data {
>>>>>       unsigned long ram_base;         /* Base address of RAM used by U-Boot */
>>>>>       unsigned long ram_top;          /* Top address of RAM used by U-Boot */
>>>>>       unsigned long relocaddr;        /* Start address of U-Boot in RAM */
>>>>> -       phys_size_t ram_size;           /* RAM size */
>>>>> +       u64 ram_size;                   /* RAM size */
>>>>>       unsigned long mon_len;          /* monitor len */
>>>>>       unsigned long irq_sp;           /* irq stack pointer */
>>>>>       unsigned long start_addr_sp;    /* start_addr_stackpointer */
>>>>> diff --git a/include/ram.h b/include/ram.h
>>>>> index 67e22d7..1fe024f 100644
>>>>> --- a/include/ram.h
>>>>> +++ b/include/ram.h
>>>>> @@ -9,7 +9,14 @@
>>>>> 
>>>>> struct ram_info {
>>>>>       phys_addr_t base;
>>>>> -       size_t size;
>>>>> +       /*
>>>>> +        * We use a 64bit type for the size to correctly handle 32bit
>>>>> +        * systems with 4GB of DRAM (which would overflow a 32bit type
>>>>> +        * and read back as 0).  For 64bit systems, we assume (for now)
>>>> 
>>>> forever :-)
>>>> 
>>>>> +        * that they will always have less than 2^65 byte of DRAM
>>>>> +        * installed. -- prt
>>>> 
>>>> Is the prt your signature? I suggest dropping it.
>>> 
>>> In fact it is. But I’ll need to rewrite the entire comment anyway for the next
>>> version of this series as there’s even more functions and places that the
>>> memory size is stored in...
>>> 
>>>> 
>>>>> +        */
>>>>> +       u64 size;
>> 
>> With LPAE available in all modern ARM cores, shouldn't phys_addr_t just
>> be u64? And then we'd probably want to use that throughout the code, right?
> 
> Quite a few currently supported boards do not support LPAE, e.g. imx6.

What I'm trying to say is that we probably want to make phys_addr_t be u64 when CONFIG_LPAE is set.

Alex

> 
> 
> live well,
>  vagrant

[U-Boot] [PATCH 1/9] dm: allow 4GB of DRAM on 32bit systems

[Lokesh Vutla]

On Sunday 02 September 2018 11:19 PM, Alexander Graf wrote:
> 
> 
>> Am 02.09.2018 um 18:04 schrieb Vagrant Cascadian <vagrant@debian.org>:
>>
>>> On 2018-09-02, Alexander Graf <agraf@suse.de> wrote:
>>>> On 02.08.18 23:31, Dr. Philipp Tomsich wrote:
>>>>
>>>>> On 2 Aug 2018, at 22:36, Simon Glass <sjg@chromium.org> wrote:
>>>>>
>>>>> On 26 July 2018 at 07:59, Philipp Tomsich
>>>>> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> wrote:
>>>>>> Even on 32bit systems a full 4GB of DRAM may be installed and reported
>>>>>> by the DRAM controller.  Whether these 4GB are larger available
>>>>>> depends on the size/configuration of address decoding windows and
>>>>>> architectural features (e.g. consider a hypothetical architecture that
>>>>>> uses dedicated instructions to access the 'memory-mapped' peripheral
>>>>>> IO ranges).  In U-Boot, the available DRAM, as reported by the
>>>>>> device-model is independent of the accessible DRAM (i.e. adjusted top
>>>>>> and effective size).
>>>>>>
>>>>>> This was first reported against the RK3288, which may have 4GB DRAM
>>>>>> attached, but will have a small (e.g. 128MB) window not accessible due
>>>>>> to address decoding limitations.
>>>>>>
>>>>>> The solution is to increase the types used for storing the ram_size to
>>>>>> have at least 33 bits even on 32bit systems... i.e. we need to use a
>>>>>> u64 for these always (this was previously only the case for
>>>>>> PHYS_64BIT, which will have unwanted side-effects for 32bit systems
>>>>>> and would require more intrusive changes).
>>>>>>
>>>>>> This commit changes the size-field in 'struct ram' and the ram_size in
>>>>>> 'gd' to be a u64.
>>>>>>
>>>>>> Reported-by: Marty E. Plummer <hanetzer@startmail.com>
>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>> ---
>>>>>>
>>>>>> include/asm-generic/global_data.h | 2 +-
>>>>>> include/ram.h                     | 9 ++++++++-
>>>>>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>>>>>
>>>>>
>>>>> Reviewed-by: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>
>>>>>
>>>>>> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
>>>>>> index 0fd4900..f3d687c 100644
>>>>>> --- a/include/asm-generic/global_data.h
>>>>>> +++ b/include/asm-generic/global_data.h
>>>>>> @@ -55,7 +55,7 @@ typedef struct global_data {
>>>>>>       unsigned long ram_base;         /* Base address of RAM used by U-Boot */
>>>>>>       unsigned long ram_top;          /* Top address of RAM used by U-Boot */
>>>>>>       unsigned long relocaddr;        /* Start address of U-Boot in RAM */
>>>>>> -       phys_size_t ram_size;           /* RAM size */
>>>>>> +       u64 ram_size;                   /* RAM size */
>>>>>>       unsigned long mon_len;          /* monitor len */
>>>>>>       unsigned long irq_sp;           /* irq stack pointer */
>>>>>>       unsigned long start_addr_sp;    /* start_addr_stackpointer */
>>>>>> diff --git a/include/ram.h b/include/ram.h
>>>>>> index 67e22d7..1fe024f 100644
>>>>>> --- a/include/ram.h
>>>>>> +++ b/include/ram.h
>>>>>> @@ -9,7 +9,14 @@
>>>>>>
>>>>>> struct ram_info {
>>>>>>       phys_addr_t base;
>>>>>> -       size_t size;
>>>>>> +       /*
>>>>>> +        * We use a 64bit type for the size to correctly handle 32bit
>>>>>> +        * systems with 4GB of DRAM (which would overflow a 32bit type
>>>>>> +        * and read back as 0).  For 64bit systems, we assume (for now)
>>>>>
>>>>> forever :-)
>>>>>
>>>>>> +        * that they will always have less than 2^65 byte of DRAM
>>>>>> +        * installed. -- prt
>>>>>
>>>>> Is the prt your signature? I suggest dropping it.
>>>>
>>>> In fact it is. But I’ll need to rewrite the entire comment anyway for the next
>>>> version of this series as there’s even more functions and places that the
>>>> memory size is stored in...
>>>>
>>>>>
>>>>>> +        */
>>>>>> +       u64 size;
>>>
>>> With LPAE available in all modern ARM cores, shouldn't phys_addr_t just
>>> be u64? And then we'd probably want to use that throughout the code, right?
>>
>> Quite a few currently supported boards do not support LPAE, e.g. imx6.
> 
> What I'm trying to say is that we probably want to make phys_addr_t be u64 when CONFIG_LPAE is set.

That's right. Enabling PHYS_64BIT should be sufficient. Based on this
phys_addr_t should be set to u32 or u64. arm already does that[1].

[1]
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/types.h;h=9af7353f0866f05dbe298a603d52d90e9c8e6d28;hb=HEAD


Thanks and regards,
Lokesh

[U-Boot] [PATCH 1/9] dm: allow 4GB of DRAM on 32bit systems

[Simon Glass]

Hi,

On 3 September 2018 at 16:29, Lokesh Vutla <lokeshvutla@ti.com> wrote:
>
>
> On Sunday 02 September 2018 11:19 PM, Alexander Graf wrote:
>>
>>
>>> Am 02.09.2018 um 18:04 schrieb Vagrant Cascadian <vagrant@debian.org>:
>>>
>>>> On 2018-09-02, Alexander Graf <agraf@suse.de> wrote:
>>>>> On 02.08.18 23:31, Dr. Philipp Tomsich wrote:
>>>>>
>>>>>> On 2 Aug 2018, at 22:36, Simon Glass <sjg@chromium.org> wrote:
>>>>>>
>>>>>> On 26 July 2018 at 07:59, Philipp Tomsich
>>>>>> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> wrote:
>>>>>>> Even on 32bit systems a full 4GB of DRAM may be installed and reported
>>>>>>> by the DRAM controller.  Whether these 4GB are larger available
>>>>>>> depends on the size/configuration of address decoding windows and
>>>>>>> architectural features (e.g. consider a hypothetical architecture that
>>>>>>> uses dedicated instructions to access the 'memory-mapped' peripheral
>>>>>>> IO ranges).  In U-Boot, the available DRAM, as reported by the
>>>>>>> device-model is independent of the accessible DRAM (i.e. adjusted top
>>>>>>> and effective size).
>>>>>>>
>>>>>>> This was first reported against the RK3288, which may have 4GB DRAM
>>>>>>> attached, but will have a small (e.g. 128MB) window not accessible due
>>>>>>> to address decoding limitations.
>>>>>>>
>>>>>>> The solution is to increase the types used for storing the ram_size to
>>>>>>> have at least 33 bits even on 32bit systems... i.e. we need to use a
>>>>>>> u64 for these always (this was previously only the case for
>>>>>>> PHYS_64BIT, which will have unwanted side-effects for 32bit systems
>>>>>>> and would require more intrusive changes).
>>>>>>>
>>>>>>> This commit changes the size-field in 'struct ram' and the ram_size in
>>>>>>> 'gd' to be a u64.
>>>>>>>
>>>>>>> Reported-by: Marty E. Plummer <hanetzer@startmail.com>
>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>> ---
>>>>>>>
>>>>>>> include/asm-generic/global_data.h | 2 +-
>>>>>>> include/ram.h                     | 9 ++++++++-
>>>>>>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>
>>>>>> Reviewed-by: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>
>>>>>>
>>>>>>> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
>>>>>>> index 0fd4900..f3d687c 100644
>>>>>>> --- a/include/asm-generic/global_data.h
>>>>>>> +++ b/include/asm-generic/global_data.h
>>>>>>> @@ -55,7 +55,7 @@ typedef struct global_data {
>>>>>>>       unsigned long ram_base;         /* Base address of RAM used by U-Boot */
>>>>>>>       unsigned long ram_top;          /* Top address of RAM used by U-Boot */
>>>>>>>       unsigned long relocaddr;        /* Start address of U-Boot in RAM */
>>>>>>> -       phys_size_t ram_size;           /* RAM size */
>>>>>>> +       u64 ram_size;                   /* RAM size */
>>>>>>>       unsigned long mon_len;          /* monitor len */
>>>>>>>       unsigned long irq_sp;           /* irq stack pointer */
>>>>>>>       unsigned long start_addr_sp;    /* start_addr_stackpointer */
>>>>>>> diff --git a/include/ram.h b/include/ram.h
>>>>>>> index 67e22d7..1fe024f 100644
>>>>>>> --- a/include/ram.h
>>>>>>> +++ b/include/ram.h
>>>>>>> @@ -9,7 +9,14 @@
>>>>>>>
>>>>>>> struct ram_info {
>>>>>>>       phys_addr_t base;
>>>>>>> -       size_t size;
>>>>>>> +       /*
>>>>>>> +        * We use a 64bit type for the size to correctly handle 32bit
>>>>>>> +        * systems with 4GB of DRAM (which would overflow a 32bit type
>>>>>>> +        * and read back as 0).  For 64bit systems, we assume (for now)
>>>>>>
>>>>>> forever :-)
>>>>>>
>>>>>>> +        * that they will always have less than 2^65 byte of DRAM
>>>>>>> +        * installed. -- prt
>>>>>>
>>>>>> Is the prt your signature? I suggest dropping it.
>>>>>
>>>>> In fact it is. But I’ll need to rewrite the entire comment anyway for the next
>>>>> version of this series as there’s even more functions and places that the
>>>>> memory size is stored in...
>>>>>
>>>>>>
>>>>>>> +        */
>>>>>>> +       u64 size;
>>>>
>>>> With LPAE available in all modern ARM cores, shouldn't phys_addr_t just
>>>> be u64? And then we'd probably want to use that throughout the code, right?
>>>
>>> Quite a few currently supported boards do not support LPAE, e.g. imx6.
>>
>> What I'm trying to say is that we probably want to make phys_addr_t be u64 when CONFIG_LPAE is set.
>
> That's right. Enabling PHYS_64BIT should be sufficient. Based on this
> phys_addr_t should be set to u32 or u64. arm already does that[1].
>
> [1]
> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/types.h;h=9af7353f0866f05dbe298a603d52d90e9c8e6d28;hb=HEAD
>

Yes I agree. So will this patch be changed?

Regards,
Simon

[U-Boot] [PATCH 1/9] dm: allow 4GB of DRAM on 32bit systems

[Philipp Tomsich]

> On 14.09.2018, at 12:53, Simon Glass <sjg@chromium.org> wrote:
> 
> Hi,
> 
> On 3 September 2018 at 16:29, Lokesh Vutla <lokeshvutla@ti.com> wrote:
>> 
>> 
>> On Sunday 02 September 2018 11:19 PM, Alexander Graf wrote:
>>> 
>>> 
>>>> Am 02.09.2018 um 18:04 schrieb Vagrant Cascadian <vagrant@debian.org>:
>>>> 
>>>>> On 2018-09-02, Alexander Graf <agraf@suse.de> wrote:
>>>>>> On 02.08.18 23:31, Dr. Philipp Tomsich wrote:
>>>>>> 
>>>>>>> On 2 Aug 2018, at 22:36, Simon Glass <sjg@chromium.org> wrote:
>>>>>>> 
>>>>>>> On 26 July 2018 at 07:59, Philipp Tomsich
>>>>>>> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> wrote:
>>>>>>>> Even on 32bit systems a full 4GB of DRAM may be installed and reported
>>>>>>>> by the DRAM controller.  Whether these 4GB are larger available
>>>>>>>> depends on the size/configuration of address decoding windows and
>>>>>>>> architectural features (e.g. consider a hypothetical architecture that
>>>>>>>> uses dedicated instructions to access the 'memory-mapped' peripheral
>>>>>>>> IO ranges).  In U-Boot, the available DRAM, as reported by the
>>>>>>>> device-model is independent of the accessible DRAM (i.e. adjusted top
>>>>>>>> and effective size).
>>>>>>>> 
>>>>>>>> This was first reported against the RK3288, which may have 4GB DRAM
>>>>>>>> attached, but will have a small (e.g. 128MB) window not accessible due
>>>>>>>> to address decoding limitations.
>>>>>>>> 
>>>>>>>> The solution is to increase the types used for storing the ram_size to
>>>>>>>> have at least 33 bits even on 32bit systems... i.e. we need to use a
>>>>>>>> u64 for these always (this was previously only the case for
>>>>>>>> PHYS_64BIT, which will have unwanted side-effects for 32bit systems
>>>>>>>> and would require more intrusive changes).
>>>>>>>> 
>>>>>>>> This commit changes the size-field in 'struct ram' and the ram_size in
>>>>>>>> 'gd' to be a u64.
>>>>>>>> 
>>>>>>>> Reported-by: Marty E. Plummer <hanetzer@startmail.com>
>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>>> ---
>>>>>>>> 
>>>>>>>> include/asm-generic/global_data.h | 2 +-
>>>>>>>> include/ram.h                     | 9 ++++++++-
>>>>>>>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>>>>>>> 
>>>>>>> 
>>>>>>> Reviewed-by: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>
>>>>>>> 
>>>>>>>> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
>>>>>>>> index 0fd4900..f3d687c 100644
>>>>>>>> --- a/include/asm-generic/global_data.h
>>>>>>>> +++ b/include/asm-generic/global_data.h
>>>>>>>> @@ -55,7 +55,7 @@ typedef struct global_data {
>>>>>>>>      unsigned long ram_base;         /* Base address of RAM used by U-Boot */
>>>>>>>>      unsigned long ram_top;          /* Top address of RAM used by U-Boot */
>>>>>>>>      unsigned long relocaddr;        /* Start address of U-Boot in RAM */
>>>>>>>> -       phys_size_t ram_size;           /* RAM size */
>>>>>>>> +       u64 ram_size;                   /* RAM size */
>>>>>>>>      unsigned long mon_len;          /* monitor len */
>>>>>>>>      unsigned long irq_sp;           /* irq stack pointer */
>>>>>>>>      unsigned long start_addr_sp;    /* start_addr_stackpointer */
>>>>>>>> diff --git a/include/ram.h b/include/ram.h
>>>>>>>> index 67e22d7..1fe024f 100644
>>>>>>>> --- a/include/ram.h
>>>>>>>> +++ b/include/ram.h
>>>>>>>> @@ -9,7 +9,14 @@
>>>>>>>> 
>>>>>>>> struct ram_info {
>>>>>>>>      phys_addr_t base;
>>>>>>>> -       size_t size;
>>>>>>>> +       /*
>>>>>>>> +        * We use a 64bit type for the size to correctly handle 32bit
>>>>>>>> +        * systems with 4GB of DRAM (which would overflow a 32bit type
>>>>>>>> +        * and read back as 0).  For 64bit systems, we assume (for now)
>>>>>>> 
>>>>>>> forever :-)
>>>>>>> 
>>>>>>>> +        * that they will always have less than 2^65 byte of DRAM
>>>>>>>> +        * installed. -- prt
>>>>>>> 
>>>>>>> Is the prt your signature? I suggest dropping it.
>>>>>> 
>>>>>> In fact it is. But I’ll need to rewrite the entire comment anyway for the next
>>>>>> version of this series as there’s even more functions and places that the
>>>>>> memory size is stored in...
>>>>>> 
>>>>>>> 
>>>>>>>> +        */
>>>>>>>> +       u64 size;
>>>>> 
>>>>> With LPAE available in all modern ARM cores, shouldn't phys_addr_t just
>>>>> be u64? And then we'd probably want to use that throughout the code, right?
>>>> 
>>>> Quite a few currently supported boards do not support LPAE, e.g. imx6.
>>> 
>>> What I'm trying to say is that we probably want to make phys_addr_t be u64 when CONFIG_LPAE is set.
>> 
>> That's right. Enabling PHYS_64BIT should be sufficient. Based on this
>> phys_addr_t should be set to u32 or u64. arm already does that[1].
>> 
>> [1]
>> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/types.h;h=9af7353f0866f05dbe298a603d52d90e9c8e6d28;hb=HEAD
>> 
> 
> Yes I agree. So will this patch be changed?

This patch-set needs to be revised for other reasons… 
However, PHYS_64BIT is not a valid option for some of the affected chips (e.g. if 
the address space is indeed 32bit and the address-decode is not straightforward).

E.g. Marty Plummer already reported that the RK3288 breaks when enabling PHYS_64BIT.

Thanks,
Philipp.

[U-Boot] [PATCH 1/9] dm: allow 4GB of DRAM on 32bit systems

[Rask Ingemann Lambertsen]

On 03-09-2018 at 19:59 +0530, Lokesh Vutla wrote:
> 
> On Sunday 02 September 2018 11:19 PM, Alexander Graf wrote:
> > 
> >> Am 02.09.2018 um 18:04 schrieb Vagrant Cascadian <vagrant@debian.org>:
> >>
> >>> With LPAE available in all modern ARM cores, shouldn't phys_addr_t just
> >>> be u64? And then we'd probably want to use that throughout the code, right?
> >>
> >> Quite a few currently supported boards do not support LPAE, e.g. imx6.
> > 
> > What I'm trying to say is that we probably want to make phys_addr_t be u64 when CONFIG_LPAE is set.
> 
> That's right. Enabling PHYS_64BIT should be sufficient. Based on this
> phys_addr_t should be set to u32 or u64. arm already does that[1].

That will cause warnings from at least two sunxi drivers. I forgot which one
the other was, but sunxi_mmc is one of them:

$ git grep -F -e '(u32 *)' drivers/mmc/sunxi_mmc.c
drivers/mmc/sunxi_mmc.c:	ccu_reg = (u32 *)ofnode_get_addr(args.node);
drivers/mmc/sunxi_mmc.c:		mmc_config_clk = (u32 *)ofnode_get_addr(args.node);

becase ofnode_get_addr() is declared like so:

include/dm/ofnode.h:phys_addr_t ofnode_get_addr(ofnode node);

-- 
Best regards,
Rask Ingemann Lambertsen