| Message ID | 1532613591-8444-2-git-send-email-philipp.tomsich@theobroma-systems.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Support 4GB of memory on 32bit systems | expand |
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
> On 2 Aug 2018, at 22:36, Simon Glass <sjg@chromium.org> wrote: > > On 26 July 2018 at 07:59, Philipp Tomsich > <philipp.tomsich@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@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
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@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@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
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@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@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
> 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@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@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
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@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@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
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@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@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
> 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@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@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.
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);
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 {
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(-)