[U-Boot] [patch 2/2] spl: Lightweight UBI and UBI fastmap support

[Thomas Gleixner]

On Fri, 8 Aug 2014, Tom Rini wrote:

> On Sat, Jul 05, 2014 at 09:48:13AM -0000, Thomas Gleixner wrote:
> 
> > Booting a payload out of NAND FLASH from the SPL is a crux today, as
> > it requires hard partioned FLASH. Not a brilliant idea with the
> > reliability of todays NAND FLASH chips.
> [snip]
> > +		CONFIG_SPL_UBI
> > +		Support for a lightweight UBI (fastmap) scanner and
> > +		loader
> 
> ... for the related options, please see doc/README.ubispl

Ok.
 
> > +Usage notes:
> > +
> > +In the board config file define for example:
> > +
> > +#define CONFIG_SPL_UBI
> > +#define CONFIG_SPL_UBI_MAX_VOL_LEBS	256
> > +#define CONFIG_SPL_UBI_MAX_PEB_SIZE	(256*1024)
> > +#define CONFIG_SPL_UBI_MAX_PEBS		4096
> > +#define CONFIG_SPL_UBI_VOL_IDS		8
> > +
> > +The size requirement is roughly as follows:
> > +
> > +    2k for the basic data structure
> > +  + CONFIG_SPL_UBI_VOL_IDS * CONFIG_SPL_UBI_MAX_VOL_LEBS * 8
> > +  + CONFIG_SPL_UBI_MAX_PEBS * 64
> > +  + CONFIG_SPL_UBI_MAX_PEB_SIZE * UBI_FM_MAX_BLOCKS
> > +
> > +The last one is big, but I really don't care in that stage. Real world
> > +implementations only use the first couple of blocks, but the code
> > +handles up to UBI_FM_MAX_BLOCKS.
> > +
> > +Given the above configuration example the requirement is about 5M
> > +which is usually not a problem to reserve in the RAM along with the
> > +other areas like the kernel/dts load address.
> > +
> > +So something like this will do the trick:
> > +
> > +#define SPL_FINFO_ADDR			0x80800000
> > +#define SPL_DTB_LOAD_ADDR		0x81800000
> > +#define SPL_KERNEL_LOAD_ADDR		0x82000000
> > +
> > +In the board file, implement the following:
> > +
> > +static struct ubispl_load myvolumes[] = {
> > +	{
> > +		.name		= "Kernel",
> > +		.vol_id		= 0,
> > +		.load_addr	= (void *)SPL_KERNEL_LOAD_ADDR,
> > +	},
> > +	{
> > +		.name		= "DTB",
> > +		.vol_id		= 1,
> > +		.load_addr	= (void *)SPL_DTB_LOAD_ADDR,
> > +	}
> > +};
> > +
> > +int spl_start_uboot(void)
> > +{
> > +	struct ubispl_info info;
> > +
> > +	info.ubi = (struct ubi_scan_info *) SPL_FINFO_ADDR;
> 
> OK, so here's some problems with the example.  First, we can / should
> (and OK, after you posted the patch this went in) moved stack into DDR
> in some cases, so in the case of wanting to use this, DDR would have to
> be up in time for that to be a valid option, so we shoudl just do that.

Well, the scan data structure is rather large, so you don't want to
have it on stack.

> Second, we have CONFIG_SYS_SPL_ARGS_ADDR for where the DT would go and
> since Falcon supports uImages only (And if/when zImage is added, we'll
> have to sort something out generically) we don't need to hard-code in a
> load address for the kernel.

Well, the way how this works is a bit different from the standard
u-boot/uimage way.

The ubispl load code is oblivious of the payload. It just copies the
data from the volume to a RAM address and checks the integrity of the
payload with the UBI volume CRC.

I did this on purpose to avoid payload specific handling in the
transfer code.

If we want to have uimage support there we'd need to tell that to the
loader, e.g. by having a flag field in the volumes decriptor table.

That means, if the uImage flag is set we'd need to treat the first
FLASH page of the payload differently:

 1) copy it to a separate buffer
 2) decode the uImage header and extract the load address
 3) copy the kernel part which is in the first flash page from
    the separate buffer to the load address
 4) Continue with the rest

That's doable, but I'm not sure if its worth the trouble. In my
experience the load address almost never changes, so there is nothing
wrong to hardcode it in the SPL.


 
> > +	/*
> > +	 * Bad block, unrecoverable ECC error, skip the block
> > +	 */
> > +	if (res) {
> > +		ubi_dbg("Skipping bad or unreadable block %d", pnum);
> 
> Here and everywhere else, we have debug() please use that.

I kept the original ubi_dbg() macros in the code which I lifted from
mainline and wrapped them in the header to keep the changes minimal.

> > +#ifdef CHECKME
> > +	/*
> > +	 * If fastmap is leaking PEBs (must not happen), raise a
> > +	 * fat warning and fall back to scanning mode.
> > +	 * We do this here because in ubi_wl_init() it's too late
> > +	 * and we cannot fall back to scanning.
> > +	 */
> > +	if (WARN_ON(count_fastmap_pebs(ai) != ubi->peb_count -
> > +		    ai->bad_peb_count - fm->used_blocks))
> > +		goto fail_bad;
> > +#endif
> 
> Use DEBUG here?

That's a paranoia check in the UBI mainline code, which I can't keep
as I have to do the accounting a bit different. I kept this as a
reminder that we might revisit this if we feel paranoid enough, but we
can surely replace it with a proper comment:

    /*
     * The upstream code has a paranoia accounting check for leaked
     * PEBs. We cannot use the upstream code as we do not account for
     * all volumes. With enforced error injection I was able to verify
     * that even if that happens the load process will fail due to the
     * integrity check of the payload volumes. When that happens the
     * code falls back into full scanning mode.
     *
     * Upstream check:
     *	if (WARN_ON(count_fastmap_pebs(ai) != ubi->peb_count -
     *		    ai->bad_peb_count - fm->used_blocks))
     *		goto fail_bad;
     */

> > +#ifdef LATER
> > +		int image_seq;
> > +		ret = ubi_io_read_ec_hdr(ubi, pnum, ech, 0);
> > +		if (ret && ret != UBI_IO_BITFLIPS) {
> > +			ubi_err("unable to read fastmap block# %i EC (PEB: %i)",
> > +				i, pnum);
> > +			if (ret > 0)
> > +				ret = UBI_BAD_FASTMAP;
> > +			goto free_hdr;
> > +		} else if (ret == UBI_IO_BITFLIPS)
> > +			fm->to_be_tortured[i] = 1;
> > +
> > +		image_seq = be32_to_cpu(ech->image_seq);
> > +		if (!ubi->image_seq)
> > +			ubi->image_seq = image_seq;
> > +		/*
> > +		 * Older UBI implementations have image_seq set to zero, so
> > +		 * we shouldn't fail if image_seq == 0.
> > +		 */
> > +		if (image_seq && (image_seq != ubi->image_seq)) {
> > +			ubi_err("wrong image seq:%d instead of %d",
> > +				be32_to_cpu(ech->image_seq), ubi->image_seq);
> > +			ret = UBI_BAD_FASTMAP;
> > +			goto free_hdr;
> > +		}
> > +#endif
> 
> What is LATER in this context?  Missing functionality?  Or debug code?

Yes, it's missing functionality to check the image sequence. I had a
failure there in the first place and postponed it, then ran out of
time ....

Thanks,

	tglx