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;} elseaddr += 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