Hi Tom, On 11/08/11 17:21, Tom Rini wrote: > 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.
That's the problem we see in Linux OMAP... some guys don't think forward and submit stuff on a per case basis, then when it comes to a bit different case, they are trying to reuse (which is fine) and end up renaming stuff all around - generating huge diff stat. Why not just do the generic stuff from the start of it? Why wait for a new case? It is pretty simple, just don't name it POP, so it can serve w/o any confusion for more cases. > >>> 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. No, it should not. What do you think of the following: In the Makefile have only: COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o Then in the spl_pop_probe.c have this type of check: #ifndef CONFIG_SPL_BUILD # error CONFIG_SPL_OMAP3_POP_PROBE requires CONFIG_SPL_BUILD #endif This way, you require the CONFIG_SPL_OMAP3_POP_PROBE symbol be a part of the CONFIG_SPL_BUILD symbols group. > > [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. Existing examples can be wrong and as Wolfgang said, it is not a good justification, but I don't really mind, just trying to raise people awareness. > >> >>> + */ >>> + >>> +#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 > -- Regards, Igor. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot