| Message ID | 1320188059-6612-1-git-send-email-marek.vasut@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
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 = 0x20000; > + > + 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 = 0x40000; > +} 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 <linux/types.h> > > +#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
> 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 <linux/types.h> > > > > +#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
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.vasut@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 <linux/types.h> > > > > +#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
> 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
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
> 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.
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
> 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
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 0000000..d887d20 --- /dev/null +++ b/drivers/mtd/onenand/onenand_spl.c @@ -0,0 +1,151 @@ +/* + * Copyright (C) 2011 Marek Vasut <marek.vasut@gmail.com> + * + * Based on code: + * Copyright (C) 2005-2009 Samsung Electronics + * Kyungmin Park <kyungmin.park@samsung.com> + * + * 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 <common.h> +#include <asm/io.h> +#include <linux/mtd/onenand_regs.h> +#include <onenand_uboot.h> + +struct spl_onenand_data { + uint32_t pagesize; + uint32_t erasesize; +}; + +#define ONENAND_PAGES_PER_BLOCK 64 +#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 = 0x20000; + + 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 = 0x40000; +} + +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) != 0xffff)) + return 1; + + for (offset = 0; offset < pagesize; offset += 4) + buf[offset / 4] = readl(addr + offset); + + return 0; +} + +int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t len) +{ + uint32_t *addr = (uint32_t *)dst; + 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); + } + + 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; + } + + return err; +} 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 <linux/types.h> +#ifndef CONFIG_SPL_BUILD + /* 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 + #endif /* __UBOOT_ONENAND_H */ diff --git a/spl/Makefile b/spl/Makefile index d4d754d..b4001bf 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -54,6 +54,7 @@ LIBS-$(CONFIG_SPL_FAT_SUPPORT) += fs/fat/libfat.o LIBS-$(CONFIG_SPL_LIBGENERIC_SUPPORT) += lib/libgeneric.o LIBS-$(CONFIG_SPL_POWER_SUPPORT) += drivers/power/libpower.o LIBS-$(CONFIG_SPL_NAND_SUPPORT) += drivers/mtd/nand/libnand.o +LIBS-$(CONFIG_SPL_ONENAND_SUPPORT) += drivers/mtd/onenand/libonenand.o LIBS-$(CONFIG_SPL_DMA_SUPPORT) += drivers/dma/libdma.o ifeq ($(SOC),omap3)
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 <marek.vasut@gmail.com> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> Cc: Kyungmin Park <kyungmin.park@samsung.com> Cc: Scott Wood <scottwood@freescale.com> --- 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