Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL
> On 11/03/2011 11:56 AM, Marek Vasut wrote: > >> On 11/02/2011 07:15 PM, Marek Vasut wrote: > On 11/01/2011 05:54 PM, Marek Vasut wrote: > > +static void spl_onenand_get_geometry(struct spl_onenand_data *data) > > +{ > >>> > >>> [...] > >>> > > + /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */ > > + if (data.pagesize == 2048) { > > + total_pages = len / 2048; > > + page = offset / 2048; > > + total_pages += !!(len & 2047); > > + } else if (data.pagesize == 4096) { > > + total_pages = len / 4096; > > + page = offset / 4096; > > + total_pages += !!(len & 4095); > > + } > > What's wrong with DIV_ROUND_UP? It should produce smaller code than > what you've done here... > >>> > >>> It pulls in aeabi_*div* functions, which won't fit into block 0 of > >>> Onenand. > >> > >> It shouldn't do that if the divisor is a constant power of 2. The > >> compiler will turn it into a shift, just like with the other divides in > >> the above code fragment. > >> > >> You can't use DIV_ROUND_UP directly on data.pagesize, but you can use it > >> in each branch of the if statement instead of that awkward and slightly > >> more expensive !!(len & 4095) construct. > > > > Expensive in what way? > > Compare the resulting asm code. You're replacing this: > > a = (b + 4095) >> 12; > > with this: > > a = b >> 12; > if (b & 4095) > a++; > > > Either way, I don't think this matters that much. > > This is code that has to fit in 1K -- why waste instructions by writing > code in a way that is *less* readable? The size is the same (tested). I'll submit a patch with DIV_ROUND_UP, whatever. > > -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL
On 11/03/2011 11:56 AM, Marek Vasut wrote: >> On 11/02/2011 07:15 PM, Marek Vasut wrote: On 11/01/2011 05:54 PM, Marek Vasut wrote: > +static void spl_onenand_get_geometry(struct spl_onenand_data *data) > +{ >>> >>> [...] >>> > + /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */ > + if (data.pagesize == 2048) { > + total_pages = len / 2048; > + page = offset / 2048; > + total_pages += !!(len & 2047); > + } else if (data.pagesize == 4096) { > + total_pages = len / 4096; > + page = offset / 4096; > + total_pages += !!(len & 4095); > + } What's wrong with DIV_ROUND_UP? It should produce smaller code than what you've done here... >>> >>> It pulls in aeabi_*div* functions, which won't fit into block 0 of >>> Onenand. >> >> It shouldn't do that if the divisor is a constant power of 2. The >> compiler will turn it into a shift, just like with the other divides in >> the above code fragment. >> >> You can't use DIV_ROUND_UP directly on data.pagesize, but you can use it >> in each branch of the if statement instead of that awkward and slightly >> more expensive !!(len & 4095) construct. > > Expensive in what way? Compare the resulting asm code. You're replacing this: a = (b + 4095) >> 12; with this: a = b >> 12; if (b & 4095) a++; > Either way, I don't think this matters that much. This is code that has to fit in 1K -- why waste instructions by writing code in a way that is *less* readable? -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL
> On 11/02/2011 07:15 PM, Marek Vasut wrote: > >> On 11/01/2011 05:54 PM, Marek Vasut wrote: > >>> +static void spl_onenand_get_geometry(struct spl_onenand_data *data) > >>> +{ > > > > [...] > > > >>> + /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */ > >>> + if (data.pagesize == 2048) { > >>> + total_pages = len / 2048; > >>> + page = offset / 2048; > >>> + total_pages += !!(len & 2047); > >>> + } else if (data.pagesize == 4096) { > >>> + total_pages = len / 4096; > >>> + page = offset / 4096; > >>> + total_pages += !!(len & 4095); > >>> + } > >> > >> What's wrong with DIV_ROUND_UP? It should produce smaller code than > >> what you've done here... > > > > It pulls in aeabi_*div* functions, which won't fit into block 0 of > > Onenand. > > It shouldn't do that if the divisor is a constant power of 2. The > compiler will turn it into a shift, just like with the other divides in > the above code fragment. > > You can't use DIV_ROUND_UP directly on data.pagesize, but you can use it > in each branch of the if statement instead of that awkward and slightly > more expensive !!(len & 4095) construct. Expensive in what way? Either way, I don't think this matters that much. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL
On 11/02/2011 07:15 PM, Marek Vasut wrote: >> On 11/01/2011 05:54 PM, Marek Vasut wrote: >>> +static void spl_onenand_get_geometry(struct spl_onenand_data *data) >>> +{ > [...] > >>> + /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */ >>> + if (data.pagesize == 2048) { >>> + total_pages = len / 2048; >>> + page = offset / 2048; >>> + total_pages += !!(len & 2047); >>> + } else if (data.pagesize == 4096) { >>> + total_pages = len / 4096; >>> + page = offset / 4096; >>> + total_pages += !!(len & 4095); >>> + } >> >> What's wrong with DIV_ROUND_UP? It should produce smaller code than >> what you've done here... > > It pulls in aeabi_*div* functions, which won't fit into block 0 of Onenand. It shouldn't do that if the divisor is a constant power of 2. The compiler will turn it into a shift, just like with the other divides in the above code fragment. You can't use DIV_ROUND_UP directly on data.pagesize, but you can use it in each branch of the if statement instead of that awkward and slightly more expensive !!(len & 4095) construct. -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL
> Hi all, > > Marek, did you see the onenand_ipl/onenand_read.c at u-boot? It's already > implemented and used for OneNAND boot. > > How do you think migrate this file into generic SPL framework? That's basically what I did, with a little polishing. > > Thank you, > Kyungmin Park ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL
Hi all, Marek, did you see the onenand_ipl/onenand_read.c at u-boot? It's already implemented and used for OneNAND boot. How do you think migrate this file into generic SPL framework? Thank you, Kyungmin Park -Original Message- From: Marek Vasut [mailto:marek.va...@gmail.com] Sent: Thursday, November 03, 2011 9:16 AM To: Scott Wood Cc: u-boot@lists.denx.de; Albert ARIBAUD; Kyungmin Park Subject: Re: [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL > On 11/01/2011 05:54 PM, Marek Vasut wrote: > > +static void spl_onenand_get_geometry(struct spl_onenand_data *data) > > +{ [...] > > + /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */ > > + if (data.pagesize == 2048) { > > + total_pages = len / 2048; > > + page = offset / 2048; > > + total_pages += !!(len & 2047); > > + } else if (data.pagesize == 4096) { > > + total_pages = len / 4096; > > + page = offset / 4096; > > + total_pages += !!(len & 4095); > > + } > > What's wrong with DIV_ROUND_UP? It should produce smaller code than > what you've done here... It pulls in aeabi_*div* functions, which won't fit into block 0 of Onenand. > > > + for (; page <= total_pages; page++) { > > + block = page / ONENAND_PAGES_PER_BLOCK; > > + rpage = page & (ONENAND_PAGES_PER_BLOCK - 1); > > + ret = spl_onenand_read_page(block, rpage, addr, data.pagesize); > > + if (ret) { > > + total_pages++; > > + err |= 1; > > + } else > > + addr += data.pagesize / 4; > > + } > > As discussed, please retain the existing block-skipping semantics. And > if you do skip a block, that's not an error to be propagated upward (as > opposed to something like an uncorrectable ECC error). > > If one side of an if statement requires braces, both sides should have > them. > > > diff --git a/include/onenand_uboot.h b/include/onenand_uboot.h > > index 92279d5..fcb50ff 100644 > > --- a/include/onenand_uboot.h > > +++ b/include/onenand_uboot.h > > @@ -16,6 +16,8 @@ > > > > #include > > > > +#ifndefCONFIG_SPL_BUILD > > + > > Please use a space rather than a tab after #define, #ifndef, etc. > > > /* Forward declarations */ > > struct mtd_info; > > struct mtd_oob_ops; > > > > @@ -52,4 +54,10 @@ extern int flexonenand_set_boundary(struct mtd_info > > *mtd, int die, > > > > extern void s3c64xx_onenand_init(struct mtd_info *); > > extern void s3c64xx_set_width_regs(struct onenand_chip *); > > > > +#else > > + > > +int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t len); > > + > > +#endif > > Why does this need to be ifdeffed at all? We normally don't ifdef > header declarations. > > -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL
> On 11/01/2011 05:54 PM, Marek Vasut wrote: > > +static void spl_onenand_get_geometry(struct spl_onenand_data *data) > > +{ [...] > > + /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */ > > + if (data.pagesize == 2048) { > > + total_pages = len / 2048; > > + page = offset / 2048; > > + total_pages += !!(len & 2047); > > + } else if (data.pagesize == 4096) { > > + total_pages = len / 4096; > > + page = offset / 4096; > > + total_pages += !!(len & 4095); > > + } > > What's wrong with DIV_ROUND_UP? It should produce smaller code than > what you've done here... It pulls in aeabi_*div* functions, which won't fit into block 0 of Onenand. > > > + for (; page <= total_pages; page++) { > > + block = page / ONENAND_PAGES_PER_BLOCK; > > + rpage = page & (ONENAND_PAGES_PER_BLOCK - 1); > > + ret = spl_onenand_read_page(block, rpage, addr, data.pagesize); > > + if (ret) { > > + total_pages++; > > + err |= 1; > > + } else > > + addr += data.pagesize / 4; > > + } > > As discussed, please retain the existing block-skipping semantics. And > if you do skip a block, that's not an error to be propagated upward (as > opposed to something like an uncorrectable ECC error). > > If one side of an if statement requires braces, both sides should have > them. > > > diff --git a/include/onenand_uboot.h b/include/onenand_uboot.h > > index 92279d5..fcb50ff 100644 > > --- a/include/onenand_uboot.h > > +++ b/include/onenand_uboot.h > > @@ -16,6 +16,8 @@ > > > > #include > > > > +#ifndefCONFIG_SPL_BUILD > > + > > Please use a space rather than a tab after #define, #ifndef, etc. > > > /* Forward declarations */ > > struct mtd_info; > > struct mtd_oob_ops; > > > > @@ -52,4 +54,10 @@ extern int flexonenand_set_boundary(struct mtd_info > > *mtd, int die, > > > > extern void s3c64xx_onenand_init(struct mtd_info *); > > extern void s3c64xx_set_width_regs(struct onenand_chip *); > > > > +#else > > + > > +int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t len); > > + > > +#endif > > Why does this need to be ifdeffed at all? We normally don't ifdef > header declarations. > > -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL
On 11/01/2011 05:54 PM, Marek Vasut wrote: > +static void spl_onenand_get_geometry(struct spl_onenand_data *data) > +{ > + uint32_t tmp; > + uint32_t dev_id, density; > + > + /* Default geometry -- 2048b page, 128k erase block. */ > + data->pagesize = 2048; > + data->erasesize = 0x2; > + > + tmp = onenand_readw(ONENAND_REG_TECHNOLOGY); > + if (tmp) > + goto dev_4k; > + > + dev_id = onenand_readw(ONENAND_REG_DEVICE_ID); > + density = dev_id >> ONENAND_DEVICE_DENSITY_SHIFT; > + density &= ONENAND_DEVICE_DENSITY_MASK; > + > + if (density < ONENAND_DEVICE_DENSITY_4Gb) > + return; > + > + if (dev_id & ONENAND_DEVICE_IS_DDP) > + return; > + > + /* 4k device geometry -- 4096b page, 256k erase block. */ > +dev_4k: > + data->pagesize = 4096; > + data->erasesize = 0x4; > +} Drop the goto and "tmp" variable, just do: if (!onenand_readw(ONENAND_REG_TECHNOLOGY)) { dev_id = onenand_readw(ONENAND_REG_DEVICE_ID); density = dev_id >> ONENAND_DEVICE_DENSITY_SHIFT; density &= ONENAND_DEVICE_DENSITY_MASK; if (density < ONENAND_DEVICE_DENSITY_4Gb) return; if (dev_id & ONENAND_DEVICE_IS_DDP) return; } > +int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t len) Please use the same name and arguments as nand_spl_load_image() in nand_spl_simple.c. > +{ > + uint32_t *addr = (uint32_t *)dst; Why not pass it in as a pointer in the first place? I know U-Boot is unlkely to support being built as 64-bit any time soon, but why introduce gratuitous 64-bit-uncleanliness? > + struct spl_onenand_data data; > + uint32_t total_pages; > + uint32_t block; > + uint32_t page, rpage; > + int ret, err = 0; > + > + spl_onenand_get_geometry(&data); > + > + /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */ > + if (data.pagesize == 2048) { > + total_pages = len / 2048; > + page = offset / 2048; > + total_pages += !!(len & 2047); > + } else if (data.pagesize == 4096) { > + total_pages = len / 4096; > + page = offset / 4096; > + total_pages += !!(len & 4095); > + } What's wrong with DIV_ROUND_UP? It should produce smaller code than what you've done here... > + for (; page <= total_pages; page++) { > + block = page / ONENAND_PAGES_PER_BLOCK; > + rpage = page & (ONENAND_PAGES_PER_BLOCK - 1); > + ret = spl_onenand_read_page(block, rpage, addr, data.pagesize); > + if (ret) { > + total_pages++; > + err |= 1; > + } else > + addr += data.pagesize / 4; > + } As discussed, please retain the existing block-skipping semantics. And if you do skip a block, that's not an error to be propagated upward (as opposed to something like an uncorrectable ECC error). If one side of an if statement requires braces, both sides should have them. > diff --git a/include/onenand_uboot.h b/include/onenand_uboot.h > index 92279d5..fcb50ff 100644 > --- a/include/onenand_uboot.h > +++ b/include/onenand_uboot.h > @@ -16,6 +16,8 @@ > > #include > > +#ifndef CONFIG_SPL_BUILD > + Please use a space rather than a tab after #define, #ifndef, etc. > /* Forward declarations */ > struct mtd_info; > struct mtd_oob_ops; > @@ -52,4 +54,10 @@ extern int flexonenand_set_boundary(struct mtd_info *mtd, > int die, > extern void s3c64xx_onenand_init(struct mtd_info *); > extern void s3c64xx_set_width_regs(struct onenand_chip *); > > +#else > + > +int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t len); > + > +#endif Why does this need to be ifdeffed at all? We normally don't ifdef header declarations. -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL
This introduces small OneNAND loader, fitting into 1kB of space (smallest possible OneNAND RAM size). Some devices equipped with such crappy chips will use this. Signed-off-by: Marek Vasut Cc: Albert ARIBAUD Cc: Kyungmin Park Cc: Scott Wood --- drivers/mtd/onenand/Makefile |4 + drivers/mtd/onenand/onenand_spl.c | 151 + include/onenand_uboot.h |8 ++ spl/Makefile |1 + 4 files changed, 164 insertions(+), 0 deletions(-) create mode 100644 drivers/mtd/onenand/onenand_spl.c V2: Introduce spl_onenand_load_image() to load data from OneNAND in SPL diff --git a/drivers/mtd/onenand/Makefile b/drivers/mtd/onenand/Makefile index b984bd4..b090d40 100644 --- a/drivers/mtd/onenand/Makefile +++ b/drivers/mtd/onenand/Makefile @@ -25,8 +25,12 @@ include $(TOPDIR)/config.mk LIB:= $(obj)libonenand.o +ifndef CONFIG_SPL_BUILD COBJS-$(CONFIG_CMD_ONENAND):= onenand_uboot.o onenand_base.o onenand_bbt.o COBJS-$(CONFIG_SAMSUNG_ONENAND)+= samsung.o +else +COBJS-y:= onenand_spl.o +endif COBJS := $(COBJS-y) SRCS := $(COBJS:.o=.c) diff --git a/drivers/mtd/onenand/onenand_spl.c b/drivers/mtd/onenand/onenand_spl.c new file mode 100644 index 000..d887d20 --- /dev/null +++ b/drivers/mtd/onenand/onenand_spl.c @@ -0,0 +1,151 @@ +/* + * Copyright (C) 2011 Marek Vasut + * + * Based on code: + * Copyright (C) 2005-2009 Samsung Electronics + * Kyungmin Park + * + * See file CREDITS for list of people who contributed to this + * project. + * + * 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 + */ + +#include +#include +#include +#include + +struct spl_onenand_data { + uint32_tpagesize; + uint32_terasesize; +}; + +#define ONENAND_PAGES_PER_BLOCK64 +#define onenand_block_address(block) (block) +#define onenand_sector_address(page) (page << 2) +#define onenand_buffer_address() ((1 << 3) << 8) +#define onenand_bufferram_address(block) (0) + +static inline uint16_t onenand_readw(uint32_t addr) +{ + return readw(CONFIG_SYS_ONENAND_BASE + addr); +} + +static inline void onenand_writew(uint16_t value, uint32_t addr) +{ + writew(value, CONFIG_SYS_ONENAND_BASE + addr); +} + +static void spl_onenand_get_geometry(struct spl_onenand_data *data) +{ + uint32_t tmp; + uint32_t dev_id, density; + + /* Default geometry -- 2048b page, 128k erase block. */ + data->pagesize = 2048; + data->erasesize = 0x2; + + tmp = onenand_readw(ONENAND_REG_TECHNOLOGY); + if (tmp) + goto dev_4k; + + dev_id = onenand_readw(ONENAND_REG_DEVICE_ID); + density = dev_id >> ONENAND_DEVICE_DENSITY_SHIFT; + density &= ONENAND_DEVICE_DENSITY_MASK; + + if (density < ONENAND_DEVICE_DENSITY_4Gb) + return; + + if (dev_id & ONENAND_DEVICE_IS_DDP) + return; + + /* 4k device geometry -- 4096b page, 256k erase block. */ +dev_4k: + data->pagesize = 4096; + data->erasesize = 0x4; +} + +static int spl_onenand_read_page(uint32_t block, uint32_t page, + uint32_t *buf, int pagesize) +{ + const uint32_t addr = CONFIG_SYS_ONENAND_BASE + ONENAND_DATARAM; + uint32_t offset; + + onenand_writew(onenand_block_address(block), + ONENAND_REG_START_ADDRESS1); + + onenand_writew(onenand_bufferram_address(block), + ONENAND_REG_START_ADDRESS2); + + onenand_writew(onenand_sector_address(page), + ONENAND_REG_START_ADDRESS8); + + onenand_writew(onenand_buffer_address(), + ONENAND_REG_START_BUFFER); + + onenand_writew(ONENAND_INT_CLEAR, ONENAND_REG_INTERRUPT); + + onenand_writew(ONENAND_CMD_READ, ONENAND_REG_COMMAND); + + while (!(onenand_readw(ONENAND_REG_INTERRUPT) & ONENAND_INT_READ)) + continue; + + /* Check for invalid block mark */ + if (page < 2 && (onenand_readw(ONENAND_SPARERAM) != 0x)) + return 1; + + for (offset = 0; offset < pagesize; offset += 4) + buf[offset / 4] = readl(addr + offset); + + retur