On 11/07/11 22:05, Tom Rini wrote:
A number of boards are populated with a PoP chip for both DDR and NAND memory. So to determine DDR timings the NAND chip needs to be probed and mfr/id returned to the board to make decisions with. All of this code is put into spl_pop_probe.c and controlled via CONFIG_SPL_OMAP3_POP_PROBE.
I don't see how POP is different from other types of packages in terms of DRAM. The same thing can be true also for non-POP packages. What I'm saying here is, I understand the necessity of that code, but why call it POP specific? If it is not POP specific, then please call it some other way (e.g. ...DRAM_NAND_PROBE). Also, hypothetically, some other means can be used for DRAM type identification, so it will be a good thing to split it, but again it is only hypothetically and it is only my thoughts, so you don't have to...
Signed-off-by: Tom Rini trini@ti.com
arch/arm/cpu/armv7/omap3/Makefile | 3 + arch/arm/cpu/armv7/omap3/spl_pop_probe.c | 84 +++++++++++++++++++++++++++ arch/arm/include/asm/arch-omap3/sys_proto.h | 1 + 3 files changed, 88 insertions(+), 0 deletions(-) create mode 100644 arch/arm/cpu/armv7/omap3/spl_pop_probe.c
diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile index 8e85891..772f3d4 100644 --- a/arch/arm/cpu/armv7/omap3/Makefile +++ b/arch/arm/cpu/armv7/omap3/Makefile @@ -31,6 +31,9 @@ COBJS += board.o COBJS += clock.o COBJS += mem.o COBJS += sys_info.o +ifdef CONFIG_SPL_BUILD +COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o +endif
Can't CONFIG_SPL_OMAP3_..._PROBE symbol default to "no" and depend on CONFIG_SPL_BUILD, so you don't need to enclose it in #ifdef?
COBJS-$(CONFIG_EMIF4) += emif4.o COBJS-$(CONFIG_SDRC) += sdrc.o diff --git a/arch/arm/cpu/armv7/omap3/spl_pop_probe.c b/arch/arm/cpu/armv7/omap3/spl_pop_probe.c new file mode 100644 index 0000000..ca66dd9 --- /dev/null +++ b/arch/arm/cpu/armv7/omap3/spl_pop_probe.c @@ -0,0 +1,84 @@ +/*
- (C) Copyright 2011
- Texas Instruments, <www.ti.com>
- Author :
Tom Rini <trini@ti.com>
- Initial Code from:
Richard Woodruff <r-woodruff2@ti.com>
Jian Zhang <jzhang@ti.com>
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License as
- published by the Free Software Foundation; either version 2 of
- the License, or (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 59 Temple Place, Suite 330, Boston,
- MA 02111-1307 USA
The address is subject to change so probably it will be a good thing to drop the address part (but leave the rest).
- */
+#include <common.h> +#include <linux/mtd/nand.h> +#include <asm/io.h> +#include <asm/arch/sys_proto.h> +#include <asm/arch/mem.h>
+#ifdef CONFIG_SPL_BUILD
no need for this #ifdef, the whole file compilation depends on that symbol being defined.
+static struct gpmc *gpmc_config = (struct gpmc *)GPMC_BASE;
+/* nand_command: Send a flash command to the flash chip */ +static void nand_command(u8 command) +{
- writeb(command, &gpmc_config->cs[0].nand_cmd);
- if (command == NAND_CMD_RESET) {
unsigned char ret_val;nand_command(NAND_CMD_STATUS);
This recursion looks redundant. Why not just replace it with: writeb(NAND_CMD_STATUS, &gpmc_config->cs[0].nand_cmd);
do {/* Wait until ready */ret_val = readl(&gpmc_config->cs[0].nand_dat);} while ((ret_val & 0x40) != 0x40);
Can't 0x40 magic be defined to have some more understandable name? Probably kind of NAND_CMD_READY mask?
- }
+}
+/*
- Many boards ship with a PoP chip of both NAND and DDR, so we need
- probe the NAND side, very earily, to see what it says and pass this
s/earily/early/
- along to the board. The board code will then use this information
- to decide what DDR timings to use.
- */
+void identify_pop_memory(int *mfr, int *id) +{
- /* Make sure that we have setup GPMC for NAND correctly. */
- writel(M_NAND_GPMC_CONFIG1, &gpmc_config->cs[0].config1);
- writel(M_NAND_GPMC_CONFIG2, &gpmc_config->cs[0].config2);
- writel(M_NAND_GPMC_CONFIG3, &gpmc_config->cs[0].config3);
- writel(M_NAND_GPMC_CONFIG4, &gpmc_config->cs[0].config4);
- writel(M_NAND_GPMC_CONFIG5, &gpmc_config->cs[0].config5);
- writel(M_NAND_GPMC_CONFIG6, &gpmc_config->cs[0].config6);
- /* Enable the GPMC Mapping */
- writel((((GPMC_SIZE_128M & 0xF) << 8) | ((NAND_BASE >> 24) & 0x3F) |
(1 << 6)), &gpmc_config->cs[0].config7);- sdelay(2000);
- /* Issue a RESET and then READID */
- nand_command(NAND_CMD_RESET);
- nand_command(NAND_CMD_READID);
- writeb(0x0, &gpmc_config->cs[0].nand_adr);
It would be nice to have a comment, why the above is needed.
- /* Read off the manufacturer and device id. */
- *mfr = readb(&gpmc_config->cs[0].nand_dat);
- *id = readb(&gpmc_config->cs[0].nand_dat);
+} +#endif diff --git a/arch/arm/include/asm/arch-omap3/sys_proto.h b/arch/arm/include/asm/arch-omap3/sys_proto.h index a53d205..2a8b5c7 100644 --- a/arch/arm/include/asm/arch-omap3/sys_proto.h +++ b/arch/arm/include/asm/arch-omap3/sys_proto.h @@ -38,6 +38,7 @@ void per_clocks_enable(void); void memif_init(void); void sdrc_init(void); void do_sdrc_init(u32, u32); +void identify_pop_memory(int *, int *);
Same thing, what are the parameters? What is the order?
void get_board_mem_timings(u32 *, u32 *, u32 *, u32 *, u32 *); void emif4_init(void); void gpmc_init(void);