| Message ID | 8fcc0d74f8ce13bceef4c1002d3dddb43cb1e448.1412262683.git.marcel@ziswiler.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Warren |
| Headers | show |
On 10/02/2014 09:38 AM, Marcel Ziswiler wrote: > On popular request this now completes the Warren's work started for > TK1: > > aeb3fcb35956461077804720b8a252d50758d7e0 Please mention the commit description too, so people can still find the commit if it's been cherry-picked to a different branch with a different commit ID: aeb3fcb35956 ARM: tegra: Use mem size from MC rather than ODMDATA > In addition to the move of using the Tegra memory controller (MC) > register rather than ODMDATA for T20, T30 and T114 as well it further > uses the generic get_ram_size() function (see "common/memsize.c") > <supposed to be used in each and every U-Boot port>TM. I still think calling get_ram_size() is completely pointless given how Tegra works and that this code is already running from RAM. Equally, there doesn't appear to be any protection in the implementation against trashing the code that implements get_ram_size() with the RAM writes the code performs. I'd rather not call get_ram_size() at all. > Added benefit is > that it should <catch 99% of hardware related (i. e. reliably > reproducible) memory errors> as well. That's *extremely* unlikely. Perhaps it will catch a very tiny percentage of HW problems, and some number of SW configuration problems (i.e. BCT EmemCfg field mismatch with the board's actual RAM size). get_ram_size() doesn't perform anything remotely like a complete RAM test; it just writes to each power-of-two address, which skips a lot of RAM cells! Even then, I'm not sure that it's robust. One issue I immediately saw: it writes to *base then reads it back with only a CPU synchronization between and no bus clear operation (e.g. writing a different pattern to a different address). I've such tests pass that operation when there's nothing connected to the bus at all, due to capacitance in bus drivers/input buffers. Aside from that, this change looks reasonable at a quick glance, although I do hope your testing was very thorough. I personally don't have time to go through and test every board we support with this change. (As an aside, I learned something from this patch; I'd thought that our earlier SoCs didn't have a register in the EMC that indicated the RAM size, and so code *had* to use the ODMDATA to determine the RAM size. Evidently that wasn't the case, and we should have been using an EMC register all along).
On Thu, 2014-10-02 at 17:29 -0600, Stephen Warren wrote: > Please mention the commit description too, so people can still find the > commit if it's been cherry-picked to a different branch with a different > commit ID: > > aeb3fcb35956 ARM: tegra: Use mem size from MC rather than ODMDATA Will sure do. > I still think calling get_ram_size() is completely pointless given how > Tegra works and that this code is already running from RAM. Equally, > there doesn't appear to be any protection in the implementation against > trashing the code that implements get_ram_size() with the RAM writes the > code performs. I'd rather not call get_ram_size() at all. I really don't think it is that pointless given most all other ports are currently using it (see doc/README.memory-test which I quoted) but I agree to a certain extend to the claim you are making. Let me add Wolfgang CC to the discussion as the original author thereof. > That's *extremely* unlikely. Perhaps it will catch a very tiny > percentage of HW problems, It would catch stuck address lines which is the most like manufacturing issue to ever happen on the RAM side of things. But agreed for that to happen is very unlikely. In the last 10 thousand Tegra modules we produced we probably only saw one or two such failures. > and some number of SW configuration problems > (i.e. BCT EmemCfg field mismatch with the board's actual RAM size). Exactly, which is BTW how we are currently still doing things on our Apalis T30 modules which exist in a 1 GB and a 2 GB flavour both running off the exact same BCT. Downsteam so far we used custom code to detect the mirroring happening on the 1 GB variant: http://git.toradex.com/gitweb/u-boot-toradex.git/blob/refs/heads/colibri:/arch/arm/cpu/armv7/tegra-common/board.c#l100 Now using get_ram_size() automatically takes care of this plus it also takes care of cases where customers for some reason flash the wrong BCT onto say some Colibri T20 modules which unfortunately may still boot even with the wrong BCT (see above given link some 22 source lines further up)! > get_ram_size() doesn't perform anything remotely like a complete RAM > test; it just writes to each power-of-two address, which skips a lot of > RAM cells! Yes, it is NOT a complete test of the RAM part but that should also not be necessary as the vendor of that part should already have done that as part of his production testing. > Even then, I'm not sure that it's robust. One issue I > immediately saw: it writes to *base then reads it back with only a CPU > synchronization between and no bus clear operation (e.g. writing a > different pattern to a different address). I've such tests pass that > operation when there's nothing connected to the bus at all, due to > capacitance in bus drivers/input buffers. I was thinking about that as well but I guess one would probably not get that far on a Tegra in that case plus if really deemed required one could add a custom synchronisation primitive (see sync define in common/memsize.c) which so far is only done for PowerPCs. > Aside from that, this change looks reasonable at a quick glance, > although I do hope your testing was very thorough. Yes, as mentioned I tested it on a Colibri T20 256 MB V1.1B, V1.1B 2nd, V1.2A, Colibri T20 512 MB V1.1C, V1.2A, Apalis T30 1GB V1.0A, Apalis T30 2 GB V1.0B, V1.0C and Colibri T30 V1.1B, V1.1C, V1.1D and V1.1E plus on a NVIDIA Jetson TK1. Unfortunately I do not have any T114 which I could test it on (or do you happen to know how I could run mainline U-Boot on my NVIDIA Shield?). > I personally don't > have time to go through and test every board we support with this change. Understood. Would be good if somebody could give it a quick try on a Dalmore I believe you guys call your T114 dev board. > (As an aside, I learned something from this patch; I'd thought that our > earlier SoCs didn't have a register in the EMC that indicated the RAM > size, and so code *had* to use the ODMDATA to determine the RAM size. > Evidently that wasn't the case, and we should have been using an EMC > register all along). Nope, we never ever used that ODMDATA crap even on T20 albeit downstream using a slightly different approach via EMC_ADR_CFG_BASE. (As an aside, I am really honoured that after working quite intensively on that stuff for the last three and a half years I am able to pass some tricks on to the actual master of disaster (;-p).
On 10/02/2014 06:18 PM, Marcel Ziswiler wrote: > On Thu, 2014-10-02 at 17:29 -0600, Stephen Warren wrote: >> Please mention the commit description too, so people can still find the >> commit if it's been cherry-picked to a different branch with a different >> commit ID: >> >> aeb3fcb35956 ARM: tegra: Use mem size from MC rather than ODMDATA > > Will sure do. > >> I still think calling get_ram_size() is completely pointless given how >> Tegra works and that this code is already running from RAM. Equally, >> there doesn't appear to be any protection in the implementation against >> trashing the code that implements get_ram_size() with the RAM writes the >> code performs. I'd rather not call get_ram_size() at all. > > I really don't think it is that pointless given most all other ports are > currently using it (see doc/README.memory-test which I quoted) but I That could due to different use-cases (see next paragraph) or simply cargo-cult programming. Recall that on many systems, U-Boot executes from ROM initially and is tasked with initializing the RAM. On all current Tegra ports, U-Boot always executes from RAM, and the boot ROM has already fully initialized RAM. That's quite a different semantic. > agree to a certain extend to the claim you are making. Let me add > Wolfgang CC to the discussion as the original author thereof. > >> That's *extremely* unlikely. Perhaps it will catch a very tiny >> percentage of HW problems, > > It would catch stuck address lines which is the most like manufacturing > issue to ever happen on the RAM side of things. But agreed for that to > happen is very unlikely. In the last 10 thousand Tegra modules we > produced we probably only saw one or two such failures. If there were any stuck address lines, the chances of U-Boot actually executing the code far enough for the RAM size test to succeed vs fail is pretty minimal, since U-Boot is executing from RAM. >> and some number of SW configuration problems >> (i.e. BCT EmemCfg field mismatch with the board's actual RAM size). > > Exactly, which is BTW how we are currently still doing things on our > Apalis T30 modules which exist in a 1 GB and a 2 GB flavour both running > off the exact same BCT. Downsteam so far we used custom code to detect > the mirroring happening on the 1 GB variant: > > http://git.toradex.com/gitweb/u-boot-toradex.git/blob/refs/heads/colibri:/arch/arm/cpu/armv7/tegra-common/board.c#l100 > > Now using get_ram_size() automatically takes care of this plus it also > takes care of cases where customers for some reason flash the wrong BCT > onto say some Colibri T20 modules which unfortunately may still boot > even with the wrong BCT (see above given link some 22 source lines > further up)! To be honest, I would rather that U-Boot explicitly *not* second guess what the user puts into the BCT. That will make debugging other problems much harder. If the user puts the wrong BCT into flash, I want something to fail hard so they realize there's a problem and fix it early. If U-Boot just ignores what the user programmed, and silently fixes it, the user may well not know about the problem, and hence leave it unfixed for longer, which will only make it far more expensive to fix.
On Fri, 2014-10-03 at 10:01 -0600, Stephen Warren wrote: > Recall that on many systems, U-Boot executes from ROM initially and is > tasked with initializing the RAM. On all current Tegra ports, U-Boot > always executes from RAM, and the boot ROM has already fully initialized > RAM. That's quite a different semantic. Understood. > If there were any stuck address lines, the chances of U-Boot actually > executing the code far enough for the RAM size test to succeed vs fail > is pretty minimal, since U-Boot is executing from RAM. Agreed. > To be honest, I would rather that U-Boot explicitly *not* second guess > what the user puts into the BCT. That will make debugging other problems > much harder. > > If the user puts the wrong BCT into flash, I want something to fail hard > so they realize there's a problem and fix it early. If U-Boot just > ignores what the user programmed, and silently fixes it, the user may > well not know about the problem, and hence leave it unfixed for longer, > which will only make it far more expensive to fix. Unfortunately it does not do that, failing hard I mean. I can e.g. happily flash various BCTs onto various boards of ours and some will boot just fine even into Linux where certainly things start to go awry. We did have real cases where customers actually did exactly that and then were on lengthy support cases until we figured this out. But if you are absolutely against calling get_ram_size() on the Tegras which I think it at least should not hurt I can certainly re-submit with that removed. But at the end for our customers we will have to put such a check into place again.
diff --git a/arch/arm/cpu/tegra-common/board.c b/arch/arm/cpu/tegra-common/board.c index 433da09..a313061 100644 --- a/arch/arm/cpu/tegra-common/board.c +++ b/arch/arm/cpu/tegra-common/board.c @@ -9,6 +9,7 @@ #include <asm/io.h> #include <asm/arch/clock.h> #include <asm/arch/funcmux.h> +#include <asm/arch/mc.h> #include <asm/arch/tegra.h> #include <asm/arch-tegra/board.h> #include <asm/arch-tegra/pmc.h> @@ -27,55 +28,6 @@ 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; - u32 reg; - - reg = readl(&pmc->pmc_scratch20); - debug("pmc->pmc_scratch20 (ODMData) = 0x%08x\n", reg); - -#if defined(CONFIG_TEGRA20) - /* bits 30:28 in OdmData are used for RAM size on T20 */ - reg &= 0x70000000; - - switch ((reg) >> 28) { - case 1: - return 0x10000000; /* 256 MB */ - case 0: - case 2: - default: - return 0x20000000; /* 512 MB */ - case 3: - return 0x40000000; /* 1GB */ - } -#else /* Tegra30/Tegra114 */ - /* bits 31:28 in OdmData are used for RAM size on T30 */ - switch ((reg) >> 28) { - case 0: - case 1: - default: - return 0x10000000; /* 256 MB */ - case 2: - return 0x20000000; /* 512 MB */ - case 3: - return 0x30000000; /* 768 MB */ - case 4: - return 0x40000000; /* 1GB */ - case 8: - return 0x7ff00000; /* 2GB - 1MB */ - } -#endif -} -#else -#include <asm/arch/mc.h> - /* Read the RAM size directly from the memory controller */ unsigned int query_sdram_size(void) { @@ -83,12 +35,22 @@ unsigned int query_sdram_size(void) u32 size_mb; size_mb = readl(&mc->mc_emem_cfg); +#if defined(CONFIG_TEGRA20) + debug("mc->mc_emem_cfg (MEM_SIZE_KB) = 0x%08x\n", size_mb); + size_mb = get_ram_size((void *)PHYS_SDRAM_1, size_mb * 1024); +#else debug("mc->mc_emem_cfg (MEM_SIZE_MB) = 0x%08x\n", size_mb); + size_mb = get_ram_size((void *)PHYS_SDRAM_1, size_mb * 1024 * 1024); +#endif - return size_mb * 1024 * 1024; -} +#if defined(CONFIG_TEGRA30) || defined(CONFIG_TEGRA114) + /* External memory limited to 2047 MB due to IROM/HI-VEC */ + if (size_mb == 0x80000000) size_mb -= 0x100000; #endif + return size_mb; +} + int dram_init(void) { /* We do not initialise DRAM here. We just query the size */ diff --git a/arch/arm/include/asm/arch-tegra114/mc.h b/arch/arm/include/asm/arch-tegra114/mc.h new file mode 100644 index 0000000..044b1e0 --- /dev/null +++ b/arch/arm/include/asm/arch-tegra114/mc.h @@ -0,0 +1,37 @@ +/* + * (C) Copyright 2014 + * NVIDIA Corporation <www.nvidia.com> + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef _TEGRA114_MC_H_ +#define _TEGRA114_MC_H_ + +/** + * Defines the memory controller registers we need/care about + */ +struct mc_ctlr { + u32 reserved0[4]; /* offset 0x00 - 0x0C */ + u32 mc_smmu_config; /* offset 0x10 */ + u32 mc_smmu_tlb_config; /* offset 0x14 */ + u32 mc_smmu_ptc_config; /* offset 0x18 */ + u32 mc_smmu_ptb_asid; /* offset 0x1C */ + u32 mc_smmu_ptb_data; /* offset 0x20 */ + u32 reserved1[3]; /* offset 0x24 - 0x2C */ + u32 mc_smmu_tlb_flush; /* offset 0x30 */ + u32 mc_smmu_ptc_flush; /* offset 0x34 */ + u32 reserved2[6]; /* offset 0x38 - 0x4C */ + u32 mc_emem_cfg; /* offset 0x50 */ + u32 mc_emem_adr_cfg; /* offset 0x54 */ + u32 mc_emem_adr_cfg_dev0; /* offset 0x58 */ + u32 mc_emem_adr_cfg_dev1; /* offset 0x5C */ + u32 reserved3[12]; /* offset 0x60 - 0x8C */ + u32 mc_emem_arb_reserved[28]; /* offset 0x90 - 0xFC */ + u32 reserved4[338]; /* offset 0x100 - 0x644 */ + u32 mc_video_protect_bom; /* offset 0x648 */ + u32 mc_video_protect_size_mb; /* offset 0x64c */ + u32 mc_video_protect_reg_ctrl; /* offset 0x650 */ +}; + +#endif /* _TEGRA114_MC_H_ */ diff --git a/arch/arm/include/asm/arch-tegra114/tegra.h b/arch/arm/include/asm/arch-tegra114/tegra.h index 5d426b5..c3d061e 100644 --- a/arch/arm/include/asm/arch-tegra114/tegra.h +++ b/arch/arm/include/asm/arch-tegra114/tegra.h @@ -19,6 +19,7 @@ #define NV_PA_SDRAM_BASE 0x80000000 /* 0x80000000 for real T114 */ #define NV_PA_TSC_BASE 0x700F0000 /* System Counter TSC regs */ +#define NV_PA_MC_BASE 0x70019000 #include <asm/arch-tegra/tegra.h> diff --git a/arch/arm/include/asm/arch-tegra20/mc.h b/arch/arm/include/asm/arch-tegra20/mc.h new file mode 100644 index 0000000..9c6e3ff --- /dev/null +++ b/arch/arm/include/asm/arch-tegra20/mc.h @@ -0,0 +1,36 @@ +/* + * (C) Copyright 2014 + * NVIDIA Corporation <www.nvidia.com> + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef _TEGRA20_MC_H_ +#define _TEGRA20_MC_H_ + +/** + * Defines the memory controller registers we need/care about + */ +struct mc_ctlr { + u32 reserved0[3]; /* offset 0x00 - 0x08 */ + u32 mc_emem_cfg; /* offset 0x0C */ + u32 mc_emem_adr_cfg; /* offset 0x10 */ + u32 mc_emem_arb_cfg0; /* offset 0x14 */ + u32 mc_emem_arb_cfg1; /* offset 0x18 */ + u32 mc_emem_arb_cfg2; /* offset 0x1C */ + u32 reserved1; /* offset 0x20 */ + u32 mc_gart_cfg; /* offset 0x24 */ + u32 mc_gart_entry_addr; /* offset 0x28 */ + u32 mc_gart_entry_data; /* offset 0x2C */ + u32 mc_gart_error_req; /* offset 0x30 */ + u32 mc_gart_error_addr; /* offset 0x34 */ + u32 reserved2; /* offset 0x38 */ + u32 mc_timeout_ctrl; /* offset 0x3C */ + u32 reserved3[6]; /* offset 0x40 - 0x54 */ + u32 mc_decerr_emem_others_status; /* offset 0x58 */ + u32 mc_decerr_emem_others_adr; /* offset 0x5C */ + u32 reserved4[40]; /* offset 0x60 - 0xFC */ + u32 reserved5[93]; /* offset 0x100 - 0x270 */ +}; + +#endif /* _TEGRA20_MC_H_ */ diff --git a/arch/arm/include/asm/arch-tegra20/tegra.h b/arch/arm/include/asm/arch-tegra20/tegra.h index 18856ac..22774ab 100644 --- a/arch/arm/include/asm/arch-tegra20/tegra.h +++ b/arch/arm/include/asm/arch-tegra20/tegra.h @@ -9,6 +9,7 @@ #define _TEGRA20_H_ #define NV_PA_SDRAM_BASE 0x00000000 +#define NV_PA_MC_BASE 0x7000F000 #include <asm/arch-tegra/tegra.h> diff --git a/arch/arm/include/asm/arch-tegra30/mc.h b/arch/arm/include/asm/arch-tegra30/mc.h new file mode 100644 index 0000000..242a1fc --- /dev/null +++ b/arch/arm/include/asm/arch-tegra30/mc.h @@ -0,0 +1,38 @@ +/* + * (C) Copyright 2014 + * NVIDIA Corporation <www.nvidia.com> + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef _TEGRA30_MC_H_ +#define _TEGRA30_MC_H_ + +/** + * Defines the memory controller registers we need/care about + */ +struct mc_ctlr { + u32 reserved0[4]; /* offset 0x00 - 0x0C */ + u32 mc_smmu_config; /* offset 0x10 */ + u32 mc_smmu_tlb_config; /* offset 0x14 */ + u32 mc_smmu_ptc_config; /* offset 0x18 */ + u32 mc_smmu_ptb_asid; /* offset 0x1C */ + u32 mc_smmu_ptb_data; /* offset 0x20 */ + u32 reserved1[3]; /* offset 0x24 - 0x2C */ + u32 mc_smmu_tlb_flush; /* offset 0x30 */ + u32 mc_smmu_ptc_flush; /* offset 0x34 */ + u32 mc_smmu_asid_security; /* offset 0x38 */ + u32 reserved2[5]; /* offset 0x3C - 0x4C */ + u32 mc_emem_cfg; /* offset 0x50 */ + u32 mc_emem_adr_cfg; /* offset 0x54 */ + u32 mc_emem_adr_cfg_dev0; /* offset 0x58 */ + u32 mc_emem_adr_cfg_dev1; /* offset 0x5C */ + u32 reserved3[12]; /* offset 0x60 - 0x8C */ + u32 mc_emem_arb_reserved[28]; /* offset 0x90 - 0xFC */ + u32 reserved4[338]; /* offset 0x100 - 0x644 */ + u32 mc_video_protect_bom; /* offset 0x648 */ + u32 mc_video_protect_size_mb; /* offset 0x64c */ + u32 mc_video_protect_reg_ctrl; /* offset 0x650 */ +}; + +#endif /* _TEGRA30_MC_H_ */ diff --git a/arch/arm/include/asm/arch-tegra30/tegra.h b/arch/arm/include/asm/arch-tegra30/tegra.h index c02c5d8..9367179 100644 --- a/arch/arm/include/asm/arch-tegra30/tegra.h +++ b/arch/arm/include/asm/arch-tegra30/tegra.h @@ -17,6 +17,7 @@ #ifndef _TEGRA30_H_ #define _TEGRA30_H_ +#define NV_PA_MC_BASE 0x7000F000 #define NV_PA_SDRAM_BASE 0x80000000 /* 0x80000000 for real T30 */ #include <asm/arch-tegra/tegra.h>
On popular request this now completes the Warren's work started for TK1: aeb3fcb35956461077804720b8a252d50758d7e0 In addition to the move of using the Tegra memory controller (MC) register rather than ODMDATA for T20, T30 and T114 as well it further uses the generic get_ram_size() function (see "common/memsize.c") <supposed to be used in each and every U-Boot port>TM. Added benefit is that it should <catch 99% of hardware related (i. e. reliably reproducible) memory errors> as well. Thoroughly tested on the various Toradex line of Tegra modules available which unfortunately does not include T114 and T124 (yet at least) plus on the Jetson TK1. Based-on-work-by: Stephen Warren <swarren@nvidia.com> Based-on-work-by: Tom Warren <twarren@nvidia.com> Signed-off-by: Marcel Ziswiler <marcel@ziswiler.com> --- arch/arm/cpu/tegra-common/board.c | 64 ++++++------------------------ arch/arm/include/asm/arch-tegra114/mc.h | 37 +++++++++++++++++ arch/arm/include/asm/arch-tegra114/tegra.h | 1 + arch/arm/include/asm/arch-tegra20/mc.h | 36 +++++++++++++++++ arch/arm/include/asm/arch-tegra20/tegra.h | 1 + arch/arm/include/asm/arch-tegra30/mc.h | 38 ++++++++++++++++++ arch/arm/include/asm/arch-tegra30/tegra.h | 1 + 7 files changed, 127 insertions(+), 51 deletions(-) create mode 100644 arch/arm/include/asm/arch-tegra114/mc.h create mode 100644 arch/arm/include/asm/arch-tegra20/mc.h create mode 100644 arch/arm/include/asm/arch-tegra30/mc.h