[U-Boot] [PATCH 08/12] OMAP3 SPL: Add identify_pop_memory function

[Igor Grinberg <grinberg@compulab.co.il>]

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);

-- 
Regards,
Igor.