[U-Boot] [PATCH 1/1]: add nand_bootupdate for i.MX6 and likes
Scott Wood
oss at buserror.net
Sat Oct 8 00:43:48 CEST 2016
On Wed, 2016-10-05 at 12:57 -0700, Sergey Kubushyn wrote:
> This one adds nand_bootupdate command for i.MX6 and similar MCUs. It
> generates proper NAND boot structures (FCB, DBBT, etc) and writes those
> along with U-Boot mx image to where they belong so system would be able
> to boot off of raw NAND.
This seems like a lot of code just to write some images to various places.
What are these "NAND boot structures" and do they really need to be generated
by U-Boot, or can they be generated by a tool to produce an image that U-Boot
writes to NAND as is? I do see there are some things to be filled in based on
NAND parameters, bad blocks, etc. but can this patching up be minimized (and
the necessary parts explained in the changelog)?
> This is my THIRD attempt to get it into the main U-Boot tree. I do
> really hope it would get applied before I retire.
It helps to CC relevant people...
> Signed-off-by: Sergey Kubushyn <ksi at koi8.net>
> ---
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -571,6 +571,16 @@ config CMD_TIME
> help
> Run commands and summarize execution time.
>
> +config CMD_NAND_BOOTUPDATE
> + bool "Update NAND bootloader on iMX6 and its brethen"
> + depends on ARCH_MX6 && NAND_BOOT && CMD_NAND
Why does a hardware-specific option have a generic name?
> + help
> + Having iMX6 NAND U-Boot image in memory creates all necessary
> + structures and writes those where they belong along with that
> + U-Boot image so system is able to boot off of raw NAND. Kinda
> + like kobs-ng utility but simpler.
> +
> +
> # TODO: rename to CMD_SLEEP
> config CMD_MISC
> bool "sleep"
>
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -67,6 +67,7 @@ obj-$(CONFIG_NAND_OMAP_GPMC) += omap_gpmc.o
> obj-$(CONFIG_NAND_OMAP_ELM) += omap_elm.o
> obj-$(CONFIG_NAND_PLAT) += nand_plat.o
> obj-$(CONFIG_NAND_SUNXI) += sunxi_nand.o
> +obj-$(CONFIG_CMD_NAND_BOOTUPDATE) += mxs_nand_bootupdate.o
This looks like your mailer mangled the whitespace in the patch, or else it
was generated strangely... Why not use git to generate patches? That would
also help with conflicts due to the SHA1 info.
>
> else # minimal SPL drivers
>
> --- a/drivers/mtd/nand/mxs_nand.c
> +++ b/drivers/mtd/nand/mxs_nand.c
> @@ -26,6 +26,7 @@
> #include <asm/imx-common/regs-gpmi.h>
> #include <asm/arch/sys_proto.h>
> #include <asm/imx-common/dma.h>
> +#include <asm/imx-common/mxs_nand.h>
>
> #define MXS_NAND_DMA_DESCRIPTOR_COUNT 4
>
> @@ -150,7 +151,7 @@ static uint32_t mxs_nand_aux_status_offset(void)
> return (MXS_NAND_METADATA_SIZE + 0x3) & ~0x3;
> }
>
> -static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
> +uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
> uint32_t page_oob_size)
Please adjust the continuation line to align with the line you modified.
> +#ifndef CONFIG_CMD_MTDPARTS
> +#error "CONFIG_CMD_MTDPARTS is not set, mtd partition support missing"
> +#endif
If this is an error then use kconfig to express the dependency.
> +
> +static const char *uboot_tgt = "nand0,0";
This is only used in one place, and you hardcode "nand0" elsewhere, so why not
just put the string where it's used?
> +static ssize_t raw_write_page(struct mtd_info *mtd, void *buf, loff_t
> offset)
> +{
> + struct mtd_oob_ops ops;
> + ssize_t ret;
> +
> + ops.mode = MTD_OPS_RAW;
> + ops.ooboffs = 0;
> + ops.datbuf = buf;
> + ops.len = mtd->writesize;
> + ops.oobbuf = buf + mtd->writesize;
> + ops.ooblen = mtd->oobsize;
> + ret = mtd_write_oob(mtd, offset, &ops);
> +
> + return ret;
> +}
Whitespace.
Is the raw access needed because certain boot blocks use different ECC than
the rest? It would be nice to handle this by having the driver apply the
proper ECC to each region, rather than having the caller do the ECC manually.
> +/* Erase entire U-Boot partition */
> +static int mxs_nand_uboot_erase(struct mtd_info *mtd, struct part_info
> *part)
> +{
> + uint64_t offset = 0;
> + struct erase_info erase;
> + int len = part->size;
> + int ret;
> +
> + while (len > 0) {
> + pr_debug("erasing at 0x%08llx\n", offset);
> + if (mtd_block_isbad(mtd, offset)) {
> + pr_debug("erase skip block @ 0x%08llx\n", offset);
> + offset += mtd->erasesize;
> + continue;
> + }
> +
> + memset(&erase, 0, sizeof(erase));
> + erase.addr = offset;
> + erase.len = mtd->erasesize;
> +
> + ret = mtd_erase(mtd, &erase);
> + if (ret)
> + return ret;
> +
> + offset += mtd->erasesize;
> + len -= mtd->erasesize;
> + }
> +
> + return 0;
> +}
What's wrong with nand_erase_opts()?
> +
> +/* Write the U-Boot proper (2 copies) to where it belongs. */
> +/* This is U-Boot binary image itself, no FCB/DBBT here yet. */
> +static int mxs_nand_uboot_write_fw(struct mtd_info *mtd, struct
> fw_write_data *fw)
> +{
> + uint64_t offset;
> + int ret;
> + int blk;
> + size_t dummy;
> + size_t bytes_left;
> + int chunk;
> + void *p;
> +
> + bytes_left = fw->len;
> + p = fw->buf;
> + blk = fw->fw1_blk;
> + offset = blk * mtd->erasesize;
> +
> + while (bytes_left > 0) {
> + chunk = min(bytes_left, mtd->erasesize);
> +
> + pr_debug("fw1: writing %p at 0x%08llx, left 0x%08x\n",
> + p, offset, bytes_left);
> +
> + if (mtd_block_isbad(mtd, offset)) {
> + pr_debug("fw1: write skip block @ 0x%08llx\n",
> offset);
> + offset += mtd->erasesize;
> + blk++;
> + continue;
> + }
> +
> + if (blk >= fw->fw2_blk)
> + return -ENOSPC;
> +
> + ret = mtd_write(mtd, offset, chunk, &dummy, p);
> + if (ret)
> + return ret;
> +
> + offset += chunk;
> + bytes_left -= chunk;
> + p += chunk;
> + blk++;
> + }
> +
> + bytes_left = fw->len;
> + p = fw->buf;
> + blk = fw->fw2_blk;
> + offset = blk * mtd->erasesize;
> +
> + while (bytes_left > 0) {
> + chunk = min(bytes_left, mtd->erasesize);
> +
> + pr_debug("fw2: writing %p at 0x%08llx, left 0x%08x\n",
> + p, offset, bytes_left);
> +
> + if (mtd_block_isbad(mtd, offset)) {
> + pr_debug("fw2: write skip block @ 0x%08llx\n",
> offset);
> + offset += mtd->erasesize;
> + blk++;
> + continue;
> + }
> +
> + if (blk >= fw->part_blks)
> + return -ENOSPC;
> +
> + ret = mtd_write(mtd, offset, chunk, &dummy, p);
> + if (ret)
> + return ret;
> +
> + offset += chunk;
> + bytes_left -= chunk;
> + p += chunk;
> + blk++;
> + }
> +
> + return 0;
> +}
What's wrong with nand_write_skip_bad()?
> +/********************************************************************/
> +/* This is where it is all done. Takes pointer to a U-Boot image in */
> +/* RAM and image size, creates FCB/DBBT and writes everything where */
> +/* it belongs into NAND. Image must be an IMX image built for NAND. */
> +/********************************************************************/
> +static int mxs_nand_uboot_update(const void *img, size_t len)
> +{
> + int i, ret;
> +
> + size_t dummy;
> + loff_t offset = 0;
> +
> + void *fcb_raw_page;
> + void *dbbt_page;
> + void *dbbt_data_page;
> + void *ecc;
> +
> + uint32_t num_blocks_fcb_dbbt;
> + uint32_t num_blocks_fw;
> +
> + struct mtd_info *mtd;
> + struct fcb_block *fcb;
> + struct dbbt_block *dbbt;
> +
> + struct mtd_device *dev;
> + struct part_info *part;
> + u8 pnum;
> +
> + struct fw_write_data fw;
> +
> + if ((mtdparts_init() == 0) &&
> + (find_dev_and_part(uboot_tgt, &dev, &pnum, &part)
> == 0)) {
Please align continuation lines nicely and avoid unnecessary parens:
if (mtdparts_init() == 0 &&
find_dev_and_part(...)) {
...
}
> + if (dev->id->type != MTD_DEV_TYPE_NAND) {
> + puts("Not a NAND device\n");
> + return -ENODEV;
> + }
> + }
> +
> + nand_curr_device = dev->id->num;
> +
> +#ifdef CONFIG_SYS_NAND_SELECT_DEVICE
> + board_nand_select_device(nand_info[nand_curr_device].priv,
> nand_curr_device);
> +#endif
> +
> + /* Get a pointer to mtd_info for selected device */
> +
> + mtd = get_mtd_device_nm("nand0"); /* We always boot off of
> nand0 */
Why do you need to do this when you just got "dev" from find_dev_and_part()?
> +
> + if (IS_ERR(mtd)) {
> + /* Should not happen */
> + puts("No nand0 device...\n");
> + return -ENODEV;
> + }
> +
> + put_mtd_device(mtd);
Why are you calling put_mtd_device() before you're done with it?
> +
> + /* Quick and dirty check if we have 2Mbytes of good blocks in
> nand0,0 */
> + /* Not sure if it is needed at all but won't hurt so here it
> is... */
> +
> + i = 0;
> + offset = 0; /* It is the first partition so it starts at
> block 0 */
> +
> + while (offset < part->size) {
>
> + if (!mtd_block_isbad(mtd, offset)) {
> + i += mtd->erasesize;
> + }
Unnecessary {}
> + offset += mtd->erasesize;
> + }
> +
> + if (i < SZ_2M) {
> + puts("Partition too small for U-Boot!\n");
> + return -EINVAL;
> + }
If you use nand_write_skip_bad() you can pass a limit that will check this for
you.
> +
> + /* We will use 4 first blocks for FCB/DBBT copies. */
> + /* The rest of partition is split in half and used */
> + /* for two U-Boot copies. We don't care if those */
> + /* start on good or bad block - bad blocks will be */
> + /* skipped on write, ROM boot code will also skip */
> + /* bad blocks on bootup when loading U-Boot image. */
/*
* Please use standard U-Booot
* multi-line comment style.
*/
> +
> + fw.part_blks = part->size / mtd->erasesize;
> + num_blocks_fcb_dbbt = 4;
> + num_blocks_fw = (fw.part_blks - num_blocks_fcb_dbbt) / 2;
> + fw.fw1_blk = num_blocks_fcb_dbbt;
> + fw.fw2_blk = fw.fw1_blk + num_blocks_fw;
> +
> + /* OK, now create FCB structure for bootROM NAND boot */
> +
> + fcb_raw_page = kzalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
> +
> + fcb = fcb_raw_page + 12;
> + ecc = fcb_raw_page + 512 + 12;
> +
> + dbbt_page = kzalloc(mtd->writesize, GFP_KERNEL);
> + dbbt_data_page = kzalloc(mtd->writesize, GFP_KERNEL);
> + dbbt = dbbt_page;
> +
> + /* Write one additional page to make the ROM happy. */
> + /* Maybe the PagesInFirmwarex fields are really the */
> + /* number of pages - 1. kobs-ng does the same. */
> +
> + fw.len = ALIGN(len + FLASH_OFFSET_STANDARD + mtd->writesize, mtd-
> >writesize);
> + fw.buf = kzalloc(fw.len, GFP_KERNEL);
> + memcpy(fw.buf + FLASH_OFFSET_STANDARD, img, len);
> +
> + /* Erase entire partition */
> + ret = mxs_nand_uboot_erase(mtd, part);
> + if (ret)
> + goto out;
> +
> + /* Now write 2 copies of the U-Boot proper to where they belong. */
> + /* Headers (FCB, DBBT) will be generated and written after that. */
> + ret = mxs_nand_uboot_write_fw(mtd, &fw);
> + if (ret < 0)
> + goto out;
> +
> + /* Create FCB, calculate ECC (we don't/can't use hardware ECC */
> + /* here so we do it ourselves and then write _RAW_ pages. */
> +
> + fcb->Firmware1_startingPage = fw.fw1_blk * mtd->erasesize / mtd-
> >writesize;
> + fcb->Firmware2_startingPage = fw.fw2_blk * mtd->erasesize / mtd-
> >writesize;
> + fcb->PagesInFirmware1 =
> + ALIGN(len + FLASH_OFFSET_STANDARD, mtd->writesize) / mtd-
> >writesize;
> + fcb->PagesInFirmware2 = fcb->PagesInFirmware1;
> +
> + fcb_create(fcb, mtd);
> + encode_hamming_13_8(fcb, ecc, 512);
> +
> + /*
> + * Set the first and second byte of OOB data to 0xFF, not 0x00.
> These
> + * bytes are used as the Manufacturers Bad Block Marker (MBBM).
> Since
> + * the FCB is mostly written to the first page in a block, a scan
> for
> + * factory bad blocks will detect these blocks as bad, e.g. when
> + * function nand_scan_bbt() is executed to build a new bad block
> table.
> + * We will _NOT_ mark a bad block as good -- we skip the bad
> blocks.
> + */
> + memset(fcb_raw_page + mtd->writesize, 0xff, 2);
> +
> + /* Now create DBBT */
> + dbbt->Checksum = 0;
> + dbbt->FingerPrint = 0x54424244;
> + dbbt->Version = 0x01000000;
> +
> + if ((ret = dbbt_data_create(mtd, dbbt_data_page, fw.part_blks)) <
> 0)
> + goto out;
> +
> + if (ret > 0)
> + dbbt->DBBTNumOfPages = 1;
> +
> + offset = 0;
> +
> + if (mtd_block_isbad(mtd, offset)) {
> + puts("Block 0 is bad, NAND unusable\n");
> + ret = -EIO;
> + goto out;
> + }
It would be nice to check for stuff like this before erasing anything, so you
don't brick the board in the event that some software or config problem is
causing all blocks to report as bad (I've seen this happen on other hardware).
> +out:
> + kfree(dbbt_page);
> + kfree(dbbt_data_page);
> + kfree(fcb_raw_page);
> + kfree(fw.buf);
Where did this code come from that uses kfree() as anything but a
compatibility wrapper?
> +
> + return ret;
> +}
> +
> +
> +int mxs_do_nand_bootupdate(ulong addr, size_t len)
> +{
> + /* KSI: Unlock NAND first if it is locked... */
> +
> + return mxs_nand_uboot_update((const void *)addr, len);
> +}
If the function takes a pointer then have it take a pointer. Don't cast it
here.
>
> --- a/cmd/nand.c
> +++ b/cmd/nand.c
> @@ -38,6 +38,11 @@ int find_dev_and_part(const char *id, struct mtd_device
> **dev,
> u8 *part_num, struct part_info **part);
> #endif
>
> +#ifdef CONFIG_CMD_NAND_BOOTUPDATE
> +/* This comes from a separate file in drivers/mtd/nand */
> +int mxs_do_nand_bootupdate(ulong addr, size_t len);
> +#endif
Prototypes need to come from header files that are also included by the
function implementation.
> static int nand_dump(struct mtd_info *mtd, ulong off, int only_oob,
> int repeat)
> {
> @@ -372,6 +377,9 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc,
> char * const argv[])
> loff_t off, size, maxsize;
> char *cmd, *s;
> struct mtd_info *mtd;
> +#ifdef CONFIG_CMD_NAND_BOOTUPDATE
> + size_t cnt;
> +#endif
Why can't you use "size" that's already there?
> #ifdef CONFIG_SYS_NAND_QUIET
> int quiet = CONFIG_SYS_NAND_QUIET;
> #else
> @@ -777,6 +785,48 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[])
> }
> #endif
>
> +#ifdef CONFIG_CMD_NAND_BOOTUPDATE
> + if (strncmp(cmd, "bootupdate", 10) == 0) {
> +
> + if (argc < 3) {
> + /* All default values */
> + addr = getenv_ulong("fileaddr", 16, 0UL);
> + cnt = getenv_ulong("filesize", 16, 0UL);
> + }
> +
> + if (argc == 3) {
> + /* 'addr' only, file size from environment */
> + if ((addr = simple_strtoul(argv[2], NULL, 16)) ==
> 0UL)
> + addr = getenv_ulong("fileaddr", 16, 0UL);
> +
> + cnt = getenv_ulong("filesize", 16, 0UL);
> + }
> +
> + if (argc > 3) {
> + /* 'addr', 'size', and possibly more */
> + if ((addr = simple_strtoul(argv[2], NULL, 16)) ==
> 0UL)
> + addr = getenv_ulong("fileaddr", 16, 0UL);
> +
> + if ((cnt = simple_strtoul(argv[3], NULL, 16)) ==
> 0UL)
> + cnt = getenv_ulong("filesize", 16, 0UL);
> + }
Don't put assignments in if-statements.
This is more simply written as:
if (strncmp(cmd, "bootupdate", 10) == 0) {
if (argc >= 3)
addr = simple_strtoul(argv[2], NULL, 16);
else
addr = getenv_ulong("fileaddr", 16, 0UL);
if (argc >= 4)
size = simple_strtoul(argv[3], NULL, 16);
else
size = getenv_ulong("filesize", 16, 0UL);
...
}
Is the undocumented "get the value from the environment if zero is explicitly
passed" semantic really necessary? If it is, then:
if (strncmp(cmd, "bootupdate", 10) == 0) {
unsigned long val;
addr = getenv_ulong("fileaddr", 16, 0UL);
size = getenv_ulong("filesize", 16, 0UL);
if (argc >= 3) {
val = simple_strtoul(argv[2], NULL, 16);
if (addr)
addr = val;
}
if (argc >= 4) {
val = simple_strtoul(argv[3], NULL, 16);
if (val)
size = val;
}
}
> + if (addr == 0 || cnt == 0) {
> + puts("Invalid arguments to nand bootupdate!\n");
> + return 1;
> + }
Is there a reason you're more worried about these invalid values than the many
other possibilities?
> +
> + if (mxs_do_nand_bootupdate(addr, cnt)) {
The help text says "in board/SoC specific manner" -- not "hardcoded to mxs".
> @@ -798,6 +848,17 @@ static char nand_help_text[] =
> " 'addr', skipping bad blocks and dropping any pages at the
> end\n"
> " of eraseblocks that contain only 0xFF\n"
> #endif
> +#ifdef CONFIG_CMD_NAND_BOOTUPDATE
> + "nand bootupdate - [addr] [size]\n"
> + " write U-Boot into NAND in board/SoC specific manner creating
> all\n"
> + " required headers and other bits and pieces as required for
> the\n"
> + " system to be able to boot off of NAND. 'addr' is the
> address\n"
> + " where U-Boot image has been loaded at, 'size' is its size.\n"
> + " If any of 'addr'/'size' is missing it is taken from
> environment\n"
> + " for the last file loaded. U-Boot image must be of a proper
> type\n"
> + " for the target platform (only IMX image supported at the
> moment)\n"
> + " binary without U-Boot image headers (e.g. u-boot.imx file.)\n"
> +#endif
This is too verbose. Most of this info should go in a README.
-Scott
More information about the U-Boot
mailing list