[U-Boot] [PATCH] i.MX6 nand bootupdate
Sergey Kubushyn
ksi at koi8.net
Wed Jun 15 21:42:44 CEST 2016
On Wed, 15 Jun 2016, Stefano Babic wrote:
Hi,
> 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.
I will when time permits, probably tomorrow. Now let's go over other
stuff...
>> 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.
The very (and only) purpose for this function is to put U-Boot with
proper FCB/DBBT header on one's board so i.MX6 bootrom would be able to
boot off of it. There is _NO_ general case. One can _NOT_ change the way
i.MX6 (or whatever) boots up, it is in the ROM and it always reads FCB
starting from sector 0 of NAND 0. BTW, if your particular NAND chip is
not supported by that ROM you are out of luck -- it won't boot off of
that chip.
Once again -- it is _NOT_ my board, it is how the i.MX6 boot code works.
> 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 ?
It makes no sense for limited purpose U-Boot command. You need at least
2 copies of FCB/DBBT. That kobs-ng puts FCB and corresponding DBBT in 2
consecutive erase blocks (sectors) thus taking 2 sectors per FCB/DBBT
instance. I put DBBT in the same sector with FCB (exactly like BareBox
does) so I can squeeze 4 copies in the same 4 sectors kobs-ng takes for
2 copies.
Then, it is a _LIMITED_ use command so it makes no sense to do something
with a long command line and its parsing code. If somebody needs more
complex/sophisticated utility he can just put U-Boot with this simple
command, boot into their favority OS and use a utility whatever complex
or big he wants.
That is what U-Boot is about. I do also miss the mkfs.ext4 or fsck or
even just an option to delete or rename a file on ext4 but I do
understand it is not all that easy and make workarounds to live with
what I have.
>> 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.
"No SPL required" means exactly that :) It does _NOT_ mean "No SPL is
possible" -- as a matter of fact neither that added command nor i.MX6
bootrom even care what that binary payload is. As long as FCB is valid
and that binary has a proper header nobody cares what it is -- it is
simply loaded and ran.
>> 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.
It needs at least 4 sectors for 4 copies of FCB/DBBT (this is 2 copies
in kbs-ng, nbare minimum) and at least 2 sectors for each copy of the
actual binary if the latter fits in one sector (minimal guarantee we can
write it if one of the 2 sectors is bad.) In real life you need some
more for NAND. Minimum/standard sector is 128K so you need at least 1MiB
for a binary that fits in 128KiB (for bootrom you also need leading 1KiB
padding and one trailing empty page so binary must fit in 125KiB.)
If the SPL is that small, yes, one can save some space so SPL partition
can be smaller. However possible 3MiB saving on a NAND chip seem hardly
worth a bother. I can make 2 commands instead of one, say ubootupdate
and splupdate but that is hardly worth a bother. What if one wanted to
put a non-SPL U-Boot on his NAND later once it's been partitioned for
SPL (more on partition later, see below)?
> Supposed to write into mtd0 works in most cases, yes. But it should be
> better let it configurable as in kobs-ng.
And the reason to do it is...? What are you going to do with a copy on
mtd other than 0? Remember, it is not us who decide where to boot from,
it is i.MX6 bootrom. It can NOT boot off of anything but nand0. And
remember, that FCB page is not even readable with regular mtd read -- it
will fail ECC...
The only use case I could imagine would be 2 redundant NANDs with their
CS lines swapped as needed by jumpers or whatever for recovery/dualboot
purposes but you can use that very switching mechanism when writing
U-Boot into that combination...
> 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.
There is no path at all. And nothing can be easier than just issue one
command -- there are no options to match/convert, no parameters, nothing
at all. What path is needed?
Furthermore once this command is in U-Boot 99.9999% of all users would
not have to bother with it at all so they won't even know such a thing
ever existed.
>> 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.
Sure.
>> Signed-off-by: Sergey Kubushyn <ksi at 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 at 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.
That's an excerpt from original copyright. I can change it to SPDX, no
biggie. Just wanted to include a verbatim copy of original copyright.
>
>> +#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.
That means you would have to _ALWAYS_ use the full form, with address
and size. And the fact that address is always a hex but size shown after
file load is in decimal while it is still hex for [almost] any command
in U-Boot so it is very confusing and prone to user errors.
And what's wrong with using default environment variable that is
_ALWAYS_ set after file load? It is not something a user can set or
forget to in his environment, those are "system" variables.
>> +
>> +
>> + 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.
I can, no problems. But it is only useful for _SOME_ Freescale (the fact
NXP bought them recently doesn't change a fact those are Freescale SOCs)
with MXS NAND. It might happen this would apply to other devices in the
future but for now it applies nothing but those SOCs with MXS NAND.
>> + 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.
Sure. But not everything at once -- it is the first, initial submission
that can be changed/ammended/betterized later on if needed. One has to
start somewhere, right?
>> 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 ?
Yep. As well as e.g. UBI is not useful without RBTREE/LZO but you'll
have to choose those yourself. And NAND_MXS also requires APHB_DMA and
some other stuff. That, in turn, require some other stuff and it goes
ad infinitum.
BTW, the fact it is not useful without NAND_MXS is the reason why I
used "mxs-" in source/functions file names. Do you still want me to
change it?
>> 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 ?
It might be. I don't care how I access those functions but it is
probably up to them to extend the structure...
BTW it is _EXACTLY_ how it is done in barebox. They started with
putting all that code into their source but then realized it is
already there, just as a file scope in another file so they simply
exported it to avoid code duplication.
If MTD guys decided to add it to their structure I will happily take it
from there. That would make it impossible to base it on a current master
until such structure change is actually applied but there is no rush
here; I can wait :)
>> 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.
I do not either :) I considered both places and the current one had been
chosen with a coin toss :) If you want me to put it in imx-common I will
do it, no problems. Just tell me to.
> In barebox it has a better name : imx-bbu-nand-fcb.c. It says much more
> as mxs_nand_bootupdate.c.
It it not. The "bbu" part stays for "BareBox Update" and we are not
barebox, we are U-Boot. If you can come up with better file name please
tell me what it is and I will rename it. I don't care what it is called.
>> 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 at 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.
No problems, just wanted to keep a verbatim copy of original copyright...
>> +
>> +
>> +#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.
Sure but it makes life more difficult. The update needs to know what
space it has to plan accordingly and calculate where to put things. That
range is also used for DBBT scan -- it doesn't make sense to go beyond
that partition (or region) when creating DBBT because it is of now other
use, just for bootrom NAND boot.
Then there is other reason for this. If one has NAND it would've been
illogical to just put U-Boot there as Variscite guys do. The remaining
space can host quite sizable FS on it. The only one currently supported
is UBIFS and UBI requires a partition. Once we have partition anyway it
raises a question why not to use a partition instead of address range
for U-Boot?
>> +
>> +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 ?
They were useful for debugging, to show what we put into that FCB. Once
we are sure it is working correctly there is nothing more to debug.
>> +
>> +
>> +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).
As I already said "bbu" stands for "BareBox Update". Do we have to keep
it in U-Boot?
If you have a better idea for function mane please go ahead and tell
what it is. I don't care what the name is; even xyz123456 would be OK.
>> +{
>> + 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 ?
So keep it for yourself :) i.MX6 bootrom won't boot off of it so no
reason to put a bootloader there.
> As I said, U-Boot works with addresses
And what?
>> + + 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 can remove it, no problems.
>> + + 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.
OK, no problems, will steal that.
>> +
>> + 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;
> ??
!! :) It is how the FCB page is laid out and it is what makes it fail
regular BCH ECC check. It is what i.MX6 bootrom expects.
FCB page is read in the raw. And written too.
>> +
>> + 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. *
******************************************************************
More information about the U-Boot
mailing list