From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aneesh V Date: Thu, 28 Jul 2011 21:48:31 +0530 Subject: [U-Boot] [PATCH V6 3/5] nand spl: add NAND Library to new SPL In-Reply-To: <4E316C53.4000701@gmail.com> References: <1311771039-31691-1-git-send-email-simonschwarzcor@gmail.com> <1311842291-24837-1-git-send-email-simonschwarzcor@gmail.com> <1311842291-24837-4-git-send-email-simonschwarzcor@gmail.com> <4E314DF1.3090007@ti.com> <4E316C53.4000701@gmail.com> Message-ID: <4E318BD7.7010102@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Thursday 28 July 2011 07:34 PM, Simon Schwarz wrote: > Hi Aneesh, > > On 07/28/2011 01:54 PM, Aneesh V wrote: >> On Thursday 28 July 2011 02:08 PM, Simon Schwarz wrote: >>> Insert some NAND driver sources into NAND SPL library. >>> >>> Signed-off-by: Simon Schwarz >> >> [snip ..] >> >>> + >>> +int nand_curr_device = -1; >> >> Is nand_curr_device used anywhere? > > Was used in nand.c - this isn't included anymore -> deleted > >> >>> +static int nand_ecc_pos[] = CONFIG_SYS_NAND_ECCPOS; >>> +static nand_info_t info; >>> +nand_info_t nand_info[CONFIG_SYS_MAX_NAND_DEVICE]; >> >> Is nand_info used anywhere? > > Same as above -> deleted. > >> >>> +static struct nand_chip nand_chip; >> >> Is nand_chip used anywhere? I see that this definition is shadowed in >> function nand_init(). > > Deleted the double definition. > > nand_chip is used in: > - nand_command > - nand_is_bad_block > - nand_read_page > - nand_init > - nand_deselect > >> >>> + >>> +#if (CONFIG_SYS_NAND_PAGE_SIZE<= 512) >> >> [snip ..] >> >>> +/* >>> + * omap_spl_read_buf16 - [DEFAULT] read chip data into buffer >>> + * @mtd: MTD device structure >>> + * @buf: buffer to store date >> >> typo: date instead of data. >> > > see below for solution (btw. this typo comes from nand_base.c) > >>> + * @len: number of bytes to read >>> + * >>> + * Default read function for 16bit buswith >>> + * >>> + * This function is based on nand_read_buf16 from nand_base.c. This >>> version >>> + * reads 32bit not 16bit although the bus only has 16bit. >>> + */ >>> +static void omap_spl_read_buf16(struct mtd_info *mtd, uint8_t *buf, >>> int len) >>> +{ >>> + int i; >>> + struct nand_chip *chip = mtd->priv; >>> + u32 *p = (u32 *) buf; >> >> Why this variable p? > It is used to cast the 8-bit buffer variable into a 32bit one. Actually > the same is done for the 16bit implementation. (There it is the adaption > to the bus width - why 32bit here see below) >> >>> + len>>= 2; >>> + >>> + for (i = 0; i< len; i++) >>> + p[i] = readl(chip->IO_ADDR_R); >>> +} >> >> Should this function be called omap_spl_read_buf32() ? >> Or still better, should this be added as nand_read_buf32() in >> nand_base.c itself? > > Oh. There I played around with the Access Size Adaptation of the GPMC - > It is still a x16 interface - this is what the 16 refers to IMHO. But Ok. I have to admit that I am not a NAND expert and I do not understand this code well. > for sake of simplicity I will change this back to 16bit access - I don't > think that there is a big performance impact although I didn't measure it. No. If it's an OMAP specific optimization, I don't see a reason to remove it. Looks like that may actually improve performance. However, you may have to take into account of the alignment of buffer, the size requested etc. Please have a look at the implementation in drivers/mtd /nand/davinci_nand.c(although the implementation here seems to be for 8-bit devices, something similar may be possible for 16-bit) > > I cloned them because the functions in nand_base.c are static. > > My solution: deleted the cloned functions - use these from nand_base by > removing the static modifier and add them to nand.h I hope there won't be any name-space conflict due to this. best regards, Aneesh