On 11/08/2011 12:45 AM, Igor Grinberg wrote:
> 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...
Well, that gets at why I called it spl_pop_probe. If you have a POP
package, this is how you would do the probe. I can see in theory
wanting to probe NAND as a way to see board rev on a non-POP package,
but I'd like to see a real example first.
>> 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?
But then it would build for both SPL and non-SPL cases.
[snip]
>> + * 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).
Just following existing examples.
>
>> + */
>> +
>> +#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.
True, will fix, thanks.
>
>> +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);
OK, thanks.
>> + 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?
I'll see if the datasheet defines this particular bit of magic.
>
>> + }
>> +}
>> +
>> +/*
>> + * 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/
Thanks.
>> + * 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.
OK.
--
Tom
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
