[U-Boot] [PATCH] i.MX6 nand bootupdate

Stefano Babic
2016-06-15T10:14:21Z

Hi Sergey,

On 14/06/2016 20:32, Sergey Kubushyn wrote:
> Here is the initial support for writing i.MX6 NAND U-Boot into NAND
> with all FCB and DBBT stuff as required.
>

This is a very interesting feature missing in U-Boot. Up now we are
constrained to update the bootloader from user space with kobs-ng, and
this is really an improvement. Thanks for that.

Anyway, as remarked by Peng, please rebase your patch on current tree,
else we cannot test it.

> Just build U-Boot for NAND as u-boot.imx, load it somehow in RAM and
> say "nand bootupdate [addr] [size]". It will create all necessary
> structures and write everything as needed.

The whole phrase is misleading here. I guess this is the way you have to
put on your board, it is not a general case.

I am expecting to have some parameters like in kobs-ng: I set the number
of copies with --search_exponent, how is it done here ?

>
> No SPL required, just plain single stage U-Boot.

Why ? This has nothing to do. We could write SPL into the NAND as first
bootloader, and SPL will load the u-boot.img unabh?ngig von FCB.

> Supposed to write into
> first NAND partition, recommended size 4MiB.

Where is this size coming ? This depends on the number of copies (can we
have configurable as for kobs-ng ?) and the size of U-Boot. If we write
SPL, we need less.

Supposed to write into mtd0 works in most cases, yes. But it should be
better let it configurable as in kobs-ng.

One reason is to show the user an easy path from kobs-ng to u-boot - and
of course, how to change from kobs-ng to this command should be
explained into the README.

> Creates 4 redundant copies
> of boot structures in first 4 sectors and 2 redundant copies of U-Boot
> in the remaining space.
>
> Tested on a Variscite VAR-SOM-SOLO based board with i.MX6DL SoC. Will
> probably post that board support files later, when config is cleaned up
> so people could have a working NAND boot DCD portion to be able actually
> use those braindead SOMs.

ok - anyway, it looks like that Peng and me can test on other boards -
no worry about it.

>
> Signed-off-by: Sergey Kubushyn <ksi@koi8.net>
> ---
> diff --git a/arch/arm/include/asm/imx-common/mxs_nand.h
> b/arch/arm/include/asm/imx-common/mxs_nand.h
> new file mode 100644
> index 0000000..7826a9f
> --- /dev/null
> +++ b/arch/arm/include/asm/imx-common/mxs_nand.h

> @@ -0,0 +1,37 @@
> +/*
> + * Copyright (C) 2015 PHYTEC Messtechnik GmbH,
> + * Author: Stefan Christ <s.christ@phytec.de>
> + *
> + * 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.
> + */
> +

Copyright is ok, but we have SPDX in U-Boot. Change the license header.

> +#ifndef __NAND_MXS_H
> +#define __NAND_MXS_H
> +
> +/*
> + * Functions are definied in drivers/mtd/nand/nand_mxs.c. They are
> used to
> + * calculate the ECC Strength, BadBlockMarkerByte and
> BadBlockMarkerStartBit
> + * which are placed into the FCB structure. The i.MX6 ROM needs these
> + * parameters to read the firmware from NAND.
> + *
> + * The parameters depends on the pagesize and oobsize of NAND chips and
> are
> + * different for each combination. To avoid placing hardcoded values in
> the bbu
> + * update handler code, the generic calculation from the driver code is
> used.
> + */
> +
> +uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
> + uint32_t page_oob_size);
> +
> +uint32_t mxs_nand_mark_byte_offset(struct mtd_info *mtd);
> +
> +uint32_t mxs_nand_mark_bit_offset(struct mtd_info *mtd);
> +
> +#endif /* __NAND_MXS_H */
> diff --git a/cmd/nand.c b/cmd/nand.c
> index 583a18f..12e38e1 100644
> --- 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
> +
> static int nand_dump(struct mtd_info *mtd, ulong off, int only_oob,
> int repeat)
> {
> @@ -373,6 +378,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
> #ifdef CONFIG_SYS_NAND_QUIET
> int quiet = CONFIG_SYS_NAND_QUIET;
> #else
> @@ -745,6 +753,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);
> + }

Why do we stick with the environment ? Most U-Boot commands do not do
it. A user can always write a script as "bootupdate ${loadaddr}
${filesize}" if he wants, the command itself should be unaware of it.

> +
> +
> + if (addr == 0 || cnt == 0) {
> + puts("Invalid arguments to nand bootupdate!\n");
> + return 1;
> + }
> + + if (mxs_do_nand_bootupdate(addr, cnt)) {

Name is misleading. The function is not related to the NAND driver
(mxs_nand), but it is an implementation of the layout requested by NXP's
SOCs. A name related to BCB (Boot Control Block) as in manual is more
correct.

So please changes all names from mxs_ in the code if they are not
strictly related to the driver, and let mxs_ name just for functions
exported by the driver.

> + puts("NAND bootupdate failed!\n");
> + return 1;
> + }
> +
> + puts("NAND bootupdate successful\n");
> + return 0;
> + }
> +#endif
> +
> usage:
> return CMD_RET_USAGE;
> }
> @@ -766,6 +816,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
> "nand erase[.spread] [clean] off size - erase 'size' bytes "
> "from offset 'off'\n"
> " With '.spread', erase enough for given file size, otherwise,\n"

A README file in doc/ is also useful.

> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 837d397..36609e1 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_TEGRA_NAND) += tegra_nand.o
> 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_CMD_NAND_BOOTUPDATE) += mxs_nand_bootupdate.o

It will be nice to set some dependencies between CONFIG_NAND_MXS and
CONFIG_CMD_NAND_BOOTUPDATE. CONFIG_CMD_NAND_BOOTUPDATE is not useful if
CONFIG_NAND_MXS is not set, right ?

>
> else # minimal SPL drivers
>
> diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
> index 7be1f86..afeec22 100644
> --- 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
>
> @@ -145,7 +146,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)
> {
> int ecc_strength;
> @@ -221,14 +222,14 @@ static inline uint32_t
> mxs_nand_get_mark_offset(uint32_t page_data_size,
> return block_mark_bit_offset;
> }
>
> -static uint32_t mxs_nand_mark_byte_offset(struct mtd_info *mtd)
> +uint32_t mxs_nand_mark_byte_offset(struct mtd_info *mtd)
> {
> uint32_t ecc_strength;
> ecc_strength = mxs_nand_get_ecc_strength(mtd->writesize,
> mtd->oobsize);
> return mxs_nand_get_mark_offset(mtd->writesize, ecc_strength) >> 3;
> }
>
> -static uint32_t mxs_nand_mark_bit_offset(struct mtd_info *mtd)
> +uint32_t mxs_nand_mark_bit_offset(struct mtd_info *mtd)
> {
> uint32_t ecc_strength;
> ecc_strength = mxs_nand_get_ecc_strength(mtd->writesize,
> mtd->oobsize);

Added Scott as MTD maintainer: it is clear to me why, but the driver
exports its functions via the struct nand_chip (see board_nand_init).

Is it not better to extend the structure with the functions you want to
export ?

> diff --git a/drivers/mtd/nand/mxs_nand_bootupdate.c
> b/drivers/mtd/nand/mxs_nand_bootupdate.c

I do not know if this file should be here or should be in imx-common. It
is relevant just for i.MX and not for other SOCs.

In barebox it has a better name : imx-bbu-nand-fcb.c. It says much more
as mxs_nand_bootupdate.c.

> new file mode 100644
> index 0000000..9442553
> --- /dev/null
> +++ b/drivers/mtd/nand/mxs_nand_bootupdate.c

> @@ -0,0 +1,556 @@
> +/*
> + * mxs_nand_bootupdate.c - write U-Boot to MXS NAND to make it bootable
> + *
> + * Copyright (c) 2016 Sergey Kubushyn <ksi@koi8.net>
> + *
> + * Most of the source shamelesly stolen from barebox.
> + *
> + * Here is the original copyright:
> + *
> + *=== Cut ===
> + * Copyright (C) 2014 Sascha Hauer, Pengutronix
> + *
> + * 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.
> + *=== Cut ===
> + */

Let Copyright and add SPDX header.

As you ported code from another Open Spurce project, it will be nice if
you write this into the commit message, writing also from which barebox
commit you took the code, exactly how we do when we port code from Linux.

> +
> +
> +#include <common.h>
> +#include <linux/sizes.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/compat.h>
> +#include <command.h>
> +#include <console.h>
> +#include <malloc.h>
> +#include <asm/byteorder.h>
> +#include <jffs2/jffs2.h>
> +#include <nand.h>
> +#include <errno.h>
> +#include <asm/imx-common/mxs_nand.h>
> +#include <asm/imx-common/imximage.cfg>
> +
> +
> +#ifndef CONFIG_CMD_MTDPARTS
> +#error "CONFIG_CMD_MTDPARTS is not set, mtd partition support missing"
> +#endif

I am missing because this is important. U-Boot works with addresses, and
not with MTDs. The whole nand utilities take an address as parameter.
MTD is optional, and can be used or not in nand erase/read/write.

> +
> +static const char *uboot_tgt = "nand0,0";
> +
> +/* partition handling routines */
> +int mtdparts_init(void);
> +int id_parse(const char *id, const char **ret_id, u8 *dev_type, u8
> *dev_num);
> +int find_dev_and_part(const char *id, struct mtd_device **dev,
> + u8 *part_num, struct part_info **part);
> +
> +struct dbbt_block {
> + uint32_t Checksum; /* reserved on i.MX6 */
> + uint32_t FingerPrint;
> + uint32_t Version;
> + uint32_t numberBB; /* reserved on i.MX6 */
> + uint32_t DBBTNumOfPages;
> +};
> +
> +struct fcb_block {
> + uint32_t Checksum; /* First fingerprint in first byte */
> + uint32_t FingerPrint; /* 2nd fingerprint at byte 4 */
> + uint32_t Version; /* 3rd fingerprint at byte 8 */
> + uint8_t DataSetup;
> + uint8_t DataHold;
> + uint8_t AddressSetup;
> + uint8_t DSAMPLE_TIME;
> + /* These are for application use only and not for ROM. */
> + uint8_t NandTimingState;
> + uint8_t REA;
> + uint8_t RLOH;
> + uint8_t RHOH;
> + uint32_t PageDataSize; /* 2048 for 2K pages, 4096 for 4K
> pages */
> + uint32_t TotalPageSize; /* 2112 for 2K pages, 4314 for 4K
> pages */
> + uint32_t SectorsPerBlock; /* Number of 2K sections per block */
> + uint32_t NumberOfNANDs; /* Total Number of NANDs - not used
> by ROM */
> + uint32_t TotalInternalDie; /* Number of separate chips in this
> NAND */
> + uint32_t CellType; /* MLC or SLC */
> + uint32_t EccBlockNEccType; /* Type of ECC, can be one of
> BCH-0-20 */
> + uint32_t EccBlock0Size; /* Number of bytes for Block0 - BCH */
> + uint32_t EccBlockNSize; /* Block size in bytes for all
> blocks other than Block0 - BCH */
> + uint32_t EccBlock0EccType; /* Ecc level for Block 0 - BCH */
> + uint32_t MetadataBytes; /* Metadata size - BCH */
> + uint32_t NumEccBlocksPerPage; /* Number of blocks per page for
> ROM use - BCH */
> + uint32_t EccBlockNEccLevelSDK; /* Type of ECC, can be one of
> BCH-0-20 */
> + uint32_t EccBlock0SizeSDK; /* Number of bytes for Block0 - BCH */
> + uint32_t EccBlockNSizeSDK; /* Block size in bytes for all blocks
> other than Block0 - BCH */
> + uint32_t EccBlock0EccLevelSDK; /* Ecc level for Block 0 - BCH */
> + uint32_t NumEccBlocksPerPageSDK;/* Number of blocks per page for
> SDK use - BCH */
> + uint32_t MetadataBytesSDK; /* Metadata size - BCH */
> + uint32_t EraseThreshold; /* To set into BCH_MODE register */
> + uint32_t BootPatch; /* 0 for normal boot and 1 to load patch
> starting next to FCB */
> + uint32_t PatchSectors; /* Size of patch in sectors */
> + uint32_t Firmware1_startingPage;/* Firmware image starts on this
> sector */
> + uint32_t Firmware2_startingPage;/* Secondary FW Image starting
> Sector */
> + uint32_t PagesInFirmware1; /* Number of sectors in firmware
> image */
> + uint32_t PagesInFirmware2; /* Number of sector in secondary FW
> image */
> + uint32_t DBBTSearchAreaStartAddress; /* Page address where dbbt
> search area begins */
> + uint32_t BadBlockMarkerByte; /* Byte in page data that have
> manufacturer marked bad block marker, */
> + /* this will be swapped with metadata[0] to
> complete page data. */
> + uint32_t BadBlockMarkerStartBit;/* For BCH ECC sizes other than 8
> and 16 the bad block marker does not */
> + /* start at 0th bit of BadBlockMarkerByte. This
> field is used to get to */
> + /* the start bit of bad block marker byte with in
> BadBlockMarkerByte */
> + uint32_t BBMarkerPhysicalOffset;/* FCB value that gives byte offset
> for bad block marker on physical NAND page */
> + uint32_t BCHType;
> +
> + uint32_t TMTiming2_ReadLatency;
> + uint32_t TMTiming2_PreambleDelay;
> + uint32_t TMTiming2_CEDelay;
> + uint32_t TMTiming2_PostambleDelay;
> + uint32_t TMTiming2_CmdAddPause;
> + uint32_t TMTiming2_DataPause;
> + uint32_t TMSpeed;
> + uint32_t TMTiming1_BusyTimeout;
> +
> + uint32_t DISBBM; /* the flag to enable (1)/disable(0) bi swap */
> + uint32_t BBMarkerPhysicalOffsetInSpareData; /* The swap position of
> main area in spare area */
> +};
> +
> +struct fw_write_data {
> + int fw1_blk;
> + int fw2_blk;
> + int part_blks;
> + void *buf;
> + size_t len;
> +};
> +
> +#define BF_VAL(v, bf) (((v) & bf##_MASK) >> bf##_OFFSET)
> +#define GETBIT(v,n) (((v) >> (n)) & 0x1)
> +
> +
> +static uint8_t calculate_parity_13_8(uint8_t d)
> +{
> + uint8_t p = 0;
> +
> + p |= (GETBIT(d, 6) ^ GETBIT(d, 5) ^ GETBIT(d, 3) ^ GETBIT(d,
> 2)) << 0;
> + p |= (GETBIT(d, 7) ^ GETBIT(d, 5) ^ GETBIT(d, 4) ^ GETBIT(d, 2) ^
> GETBIT(d, 1)) << 1;
> + p |= (GETBIT(d, 7) ^ GETBIT(d, 6) ^ GETBIT(d, 5) ^ GETBIT(d, 1) ^
> GETBIT(d, 0)) << 2;
> + p |= (GETBIT(d, 7) ^ GETBIT(d, 4) ^ GETBIT(d, 3) ^ GETBIT(d,
> 0)) << 3;
> + p |= (GETBIT(d, 6) ^ GETBIT(d, 4) ^ GETBIT(d, 3) ^ GETBIT(d, 2) ^
> GETBIT(d, 1) ^ GETBIT(d, 0)) << 4;
> + return p;
> +}
> +
> +static void encode_hamming_13_8(void *_src, void *_ecc, size_t size)
> +{
> + int i;
> + uint8_t *src = _src;
> + uint8_t *ecc = _ecc;
> +
> + for (i = 0; i < size; i++)
> + ecc[i] = calculate_parity_13_8(src[i]);
> +}
> +
> +static uint32_t calc_chksum(void *buf, size_t size)
> +{
> + u32 chksum = 0;
> + u8 *bp = buf;
> + size_t i;
> +
> + for (i = 0; i < size; i++)
> + chksum += bp[i];
> +
> + return ~chksum;
> +}

I understand that it is for debug only - but you have removed debug
functions from barebox (dump_fcb for example). Can they be useful here,
too ?

> +
> +
> +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;
> +}
> +
> +static int fcb_create(struct fcb_block *fcb, struct mtd_info *mtd)
> +{
> + fcb->FingerPrint = 0x20424346;
> + fcb->Version = 0x01000000;
> + fcb->PageDataSize = mtd->writesize;
> + fcb->TotalPageSize = mtd->writesize + mtd->oobsize;
> + fcb->SectorsPerBlock = mtd->erasesize / mtd->writesize;
> +
> + /* Divide ECC strength by two and save the value into FCB
> structure. */
> + fcb->EccBlock0EccType =
> + mxs_nand_get_ecc_strength(mtd->writesize, mtd->oobsize) >> 1;
> +
> + fcb->EccBlockNEccType = fcb->EccBlock0EccType;
> +
> + /* Also hardcoded in kobs-ng */
> + fcb->EccBlock0Size = 0x00000200;
> + fcb->EccBlockNSize = 0x00000200;
> + fcb->DataSetup = 80;
> + fcb->DataHold = 60;
> + fcb->AddressSetup = 25;
> + fcb->DSAMPLE_TIME = 6;
> + fcb->MetadataBytes = 10;
> +
> + /* DBBT search area starts at second page on first block */
> + fcb->DBBTSearchAreaStartAddress = 1;
> +
> + fcb->BadBlockMarkerByte = mxs_nand_mark_byte_offset(mtd);
> + fcb->BadBlockMarkerStartBit = mxs_nand_mark_bit_offset(mtd);
> +
> + fcb->BBMarkerPhysicalOffset = mtd->writesize;
> +
> + fcb->NumEccBlocksPerPage = mtd->writesize / fcb->EccBlock0Size - 1;
> +
> + fcb->Checksum = calc_chksum((void *)fcb + 4, sizeof(*fcb) - 4);
> +
> + return 0;
> +}
> +
> +/* 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;
> +}
> +
> +/* 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;
> +}
> +
> +static int dbbt_data_create(struct mtd_info *mtd, void *buf, int
> num_blocks)
> +{
> + int n;
> + int n_bad_blocks = 0;
> + uint32_t *bb = buf + 0x8;
> + uint32_t *n_bad_blocksp = buf + 0x4;
> +
> + for (n = 0; n < num_blocks; n++) {
> + loff_t offset = n * mtd->erasesize;
> + if (mtd_block_isbad(mtd, offset)) {
> + n_bad_blocks++;
> + *bb = n;
> + bb++;
> + }
> + }
> +
> + *n_bad_blocksp = n_bad_blocks;
> +
> + return n_bad_blocks;
> +}
> +
> +/********************************************************************/
> +/* 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. */
> +/********************************************************************/

Multiline comment, please fix it.

> +static int mxs_nand_uboot_update(const void *img, size_t len)

I would prefer you let the function names as in original code, so we
know where they are coming from (imx_bbu_nand_update here).

> +{
> + 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)) {
> + 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 */

...and if I have nand1 ?

As I said, U-Boot works with addresses

> + + if (IS_ERR(mtd)) {
> + /* Should not happen */
> + puts("No nand0 device...\n");
> + return -ENODEV;
> + }
> + + put_mtd_device(mtd);
> +
> + /* 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... */

Yes, it hurts.

> + + 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;
> + }
> + offset += mtd->erasesize;
> + }
> + + if (i < SZ_2M) {
> + puts("Partition too small for U-Boot!\n");
> + return -EINVAL;
> + }

I try to understand your changes from barebox code - something are clear
due to the differences between the two bootloaders, some other ones not.

> +
> + /* 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. */

Multiline comment is wrong. I think the original comment in bareboc is
much more clear.

> +
> + 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;
> + }
> +
> + /* Write FCB/DBBT to first 4 blocks. Skip bad blocks if any. */
> + /* Less than 4 copies will be written if there were BBs !!! */
> + for (i = 0; i < 4; i++) {
> +
> + if (mtd_block_isbad(mtd, offset)) {
> + pr_err("Block %d is bad, skipped\n", i);
> + continue;
> + }
> +
> +
> + ret = raw_write_page(mtd, fcb_raw_page, mtd->erasesize * i);
> + if (ret)
> + goto out;
> +
> + ret = mtd_write(mtd, mtd->erasesize * i + mtd->writesize,
> + mtd->writesize, &dummy, dbbt_page);
> + if (ret)
> + goto out;
> +
> + /* DBBTNumOfPages == 0 if no bad blocks */
> + if (dbbt->DBBTNumOfPages > 0) {
> + ret = mtd_write(mtd, mtd->erasesize * i + mtd->writesize * 5,
> + mtd->writesize, &dummy, dbbt_data_page);
> + if (ret)
> + goto out;
> + }
> + }
> +
> +out:
> + kfree(dbbt_page);
> + kfree(dbbt_data_page);
> + kfree(fcb_raw_page);
> + kfree(fw.buf);
> +
> + 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);
> +}
>
> ---
> ******************************************************************
> * KSI at home KOI8 Net < > The impossible we do immediately. *
> * Las Vegas NV, USA < > Miracles require 24-hour notice. *
> ******************************************************************
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Best regards,
Stefano Babic

--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================