diff mbox

[U-Boot] ARM: tegra: Use mem size from MC rather than ODMDATA

Message ID 1404331950-4916-1-git-send-email-swarren@wwwdotorg.org
State Accepted
Delegated to: Tom Warren
Headers show

Commit Message

Stephen Warren July 2, 2014, 8:12 p.m. UTC 
From: Stephen Warren <swarren@nvidia.com>

In at least Tegra124, the Tegra memory controller (MC) has a register
that controls the memory size. Read this to determine the memory size
rather than requiring this to be redundantly encoded into the ODMDATA.
This way, changes to the BCT (i.e. MC configuration) automatically
updated SW's view of the memory size, without requiring manual changes
to the ODMDATA.

Future work potentially required:
* Clip the memory size to architectural limits; U-Boot probably doesn't
  and won't support either LPAE or Tegra's "swiss cheese" memory layout,
  at least one of which would be required for >2GB RAM.
* Subtract out any carveout required by firmware on future SoCs.

Based-on-work-by: Tom Warren <twarren@nvidia.com>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
Note: this patch depends on Bryan's/Alex's "ARM: tegra: Disable VPR",
since that introduces mc.h, which this patch relies on.
---
 arch/arm/cpu/tegra-common/board.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Wolfgang Denk July 2, 2014, 9:18 p.m. UTC | #1 
Dear Stephen Warren,

In message <1404331950-4916-1-git-send-email-swarren@wwwdotorg.org> you wrote:
> 
> In at least Tegra124, the Tegra memory controller (MC) has a register
> that controls the memory size. Read this to determine the memory size
> rather than requiring this to be redundantly encoded into the ODMDATA.
> This way, changes to the BCT (i.e. MC configuration) automatically
> updated SW's view of the memory size, without requiring manual changes
> to the ODMDATA.

Is there a specific reason for not using get_ram_size()?

Best regards,

Wolfgang Denk
Stephen Warren July 2, 2014, 9:53 p.m. UTC | #2 
On 07/02/2014 03:18 PM, Wolfgang Denk wrote:
> Dear Stephen Warren,
> 
> In message <1404331950-4916-1-git-send-email-swarren@wwwdotorg.org> you wrote:
>>
>> In at least Tegra124, the Tegra memory controller (MC) has a register
>> that controls the memory size. Read this to determine the memory size
>> rather than requiring this to be redundantly encoded into the ODMDATA.
>> This way, changes to the BCT (i.e. MC configuration) automatically
>> updated SW's view of the memory size, without requiring manual changes
>> to the ODMDATA.
> 
> Is there a specific reason for not using get_ram_size()?

Since we know the exact RAM size, we may as well simply use it directly
rather than "probing" for it.

I know that if non-existent peripheral addresses are accessed by the
CPU, the CPU or some bus hangs. I'm not sure if the same applies to
addresses within the memory window where there is not actually RAM
present on a particular board, but I'd rather not risk it by touching
them during probing.

BTW, I'm out on vacation starting tomorrow, so may not respond soon.
Wolfgang Denk July 3, 2014, 7:45 a.m. UTC | #3 
Dear Stephen,

In message <53B47F6F.1090405@wwwdotorg.org> you wrote:
>
> > Is there a specific reason for not using get_ram_size()?
> 
> Since we know the exact RAM size, we may as well simply use it directly
> rather than "probing" for it.

You _think_ you know the size, but you can never be sure that all this
RAM is actually present and working.  There has been many discussions
before why using get_ram_size() makes a lot of sense even in fixed
size RAM configurations.

Best regards,

Wolfgang Denk
Tom Rini July 3, 2014, 11:01 a.m. UTC | #4 
On Thu, Jul 03, 2014 at 09:45:52AM +0200, Wolfgang Denk wrote:
> Dear Stephen,
> 
> In message <53B47F6F.1090405@wwwdotorg.org> you wrote:
> >
> > > Is there a specific reason for not using get_ram_size()?
> > 
> > Since we know the exact RAM size, we may as well simply use it directly
> > rather than "probing" for it.
> 
> You _think_ you know the size, but you can never be sure that all this
> RAM is actually present and working.  There has been many discussions
> before why using get_ram_size() makes a lot of sense even in fixed
> size RAM configurations.

Right which is why the flow in this case is:
1) Read the place that "knows"
2) Pass that size to get_ram_size(), use returned value as what we
really know the size to be.
Stephen Warren July 24, 2014, 9 p.m. UTC | #5 
On 07/03/2014 05:01 AM, Tom Rini wrote:
> On Thu, Jul 03, 2014 at 09:45:52AM +0200, Wolfgang Denk wrote:
>> Dear Stephen,
>>
>> In message <53B47F6F.1090405@wwwdotorg.org> you wrote:
>>>
>>>> Is there a specific reason for not using get_ram_size()?
>>>
>>> Since we know the exact RAM size, we may as well simply use it directly
>>> rather than "probing" for it.
>>
>> You _think_ you know the size, but you can never be sure that all this
>> RAM is actually present and working.  There has been many discussions
>> before why using get_ram_size() makes a lot of sense even in fixed
>> size RAM configurations.
>
> Right which is why the flow in this case is:
> 1) Read the place that "knows"
> 2) Pass that size to get_ram_size(), use returned value as what we
> really know the size to be.

Wolfgang, given Tom's explanation, are you now OK with this patch? TomW 
would like clarification?

But to address your points: We really do know that the RAM size is equal 
to what this register says, since at this point in the code, U-Boot is 
already running in RAM (since our HW's boot ROM initializes RAM and 
copies U-Boot to it). Any issues with the RAM itself, either bad HW or 
incorrect configuration, would already have caused a problem just 
executing any code.
Stephen Warren July 31, 2014, 7:45 p.m. UTC | #6 
On 07/02/2014 02:12 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> In at least Tegra124, the Tegra memory controller (MC) has a register
> that controls the memory size. Read this to determine the memory size
> rather than requiring this to be redundantly encoded into the ODMDATA.
> This way, changes to the BCT (i.e. MC configuration) automatically
> updated SW's view of the memory size, without requiring manual changes
> to the ODMDATA.
>
> Future work potentially required:
> * Clip the memory size to architectural limits; U-Boot probably doesn't
>    and won't support either LPAE or Tegra's "swiss cheese" memory layout,
>    at least one of which would be required for >2GB RAM.
> * Subtract out any carveout required by firmware on future SoCs.

I noticed that this patch triggers a bug in DT relocation during 
bootm/bootz. It'd be best if the following were applied somewhere before 
this patch:

http://patchwork.ozlabs.org/patch/375408/
lib: lmb: fix overflow in __lmb_alloc_base w/ large RAM

That's because this patch changes the RAM size on Tegra124 from 2GB-1MB 
to 2GB. I believe that change is correct, it just triggers an overflow 
in U-Boot's RAM handling which needs to be fixed first.

In practice, I suspect even if this dependency is ignored, there will be 
no user-visible impact, because the default environment in Tegra U-Boot 
disables DT relocation completely. This problem would only affect 
someone who flashed/ran a U-Boot with this patch without resetting their 
environment to the default that's embedded into the new U-Boot, and 
their environment in flash was older than when we disabled DT 
relocation. Since tegra-uboot-flasher always does reset the environment, 
I expect most people would never notice this.
diff mbox

Patch

diff --git a/arch/arm/cpu/tegra-common/board.c b/arch/arm/cpu/tegra-common/board.c
index 6a6faf4b2760..433da09d10c2 100644
--- a/arch/arm/cpu/tegra-common/board.c
+++ b/arch/arm/cpu/tegra-common/board.c
@@ -27,11 +27,12 @@  enum {
 	UART_COUNT = 5,
 };
 
+#if defined(CONFIG_TEGRA20) || defined(CONFIG_TEGRA30) || \
+	defined(CONFIG_TEGRA114)
 /*
  * Boot ROM initializes the odmdata in APBDEV_PMC_SCRATCH20_0,
  * so we are using this value to identify memory size.
  */
-
 unsigned int query_sdram_size(void)
 {
 	struct pmc_ctlr *const pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
@@ -72,6 +73,21 @@  unsigned int query_sdram_size(void)
 	}
 #endif
 }
+#else
+#include <asm/arch/mc.h>
+
+/* Read the RAM size directly from the memory controller */
+unsigned int query_sdram_size(void)
+{
+	struct mc_ctlr *const mc = (struct mc_ctlr *)NV_PA_MC_BASE;
+	u32 size_mb;
+
+	size_mb = readl(&mc->mc_emem_cfg);
+	debug("mc->mc_emem_cfg (MEM_SIZE_MB) = 0x%08x\n", size_mb);
+
+	return size_mb * 1024 * 1024;
+}
+#endif
 
 int dram_init(void)
 {