[U-Boot] [PATCH][RFC] MTD resync from Linux-3.8-rc2+ (master)

[Scott Wood]

Subject line should be something like
"mtd: sync NAND and mtdcore with Linux v3.8-rc2".

What is in v3.8-rc2 that you need?  I agree with others that we should
sync from stable versions.

On 12/28/2012 05:19:17 AM, Sergey Lapin wrote:
> 
> Signed-off-by: Sergey Lapin <slapin at ossfans.org>
> ---
> 
> Warning: no testing, except for single board,
> was made with this patch.
> 
> It is just basic resync for NAND parts of MTD with some modifications  
> to mtdcore
> I tried to be as close to original as possible.

What is the code size change with this patch?

> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 543c845..ba1e22a 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -25,7 +25,9 @@ include $(TOPDIR)/config.mk
> 
>  LIB	:= $(obj)libmtd.o
> 
> -COBJS-$(CONFIG_MTD_DEVICE) += mtdcore.o
> +ifneq (,$(findstring y,$(CONFIG_MTD_DEVICE)$(CONFIG_CMD_NAND)))
> +COBJS-y += mtdcore.o
> +endif

So NAND now depends on mtdcore... what changed to make this necessary?

And shouldn't we fix board files to include both, rather than hack up  
the makefiles in this one spot?

What about the #ifdef CONFIG_MTD_DEVICE in drivers/mtd/nand/nand.c?

> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 3a81ada..1e722a1 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -9,6 +9,7 @@
> 
>  #include <linux/mtd/mtd.h>
>  #include <linux/compat.h>
> +#include <linux/mtd/partitions.h>
>  #include <ubi_uboot.h>
> 
>  struct mtd_info *mtd_table[MAX_MTD_DEVICES];
> @@ -32,6 +33,30 @@ int add_mtd_device(struct mtd_info *mtd)
>  			   our caller is still holding us here. So none
>  			   of this try_ nonsense, and no bitching about  
> it
>  			   either. :) */
> +			/* default value if not set by driver */
> +			if (mtd->bitflip_threshold == 0)
> +				mtd->bitflip_threshold =  
> mtd->ecc_strength;
> +
> +			if (is_power_of_2(mtd->erasesize))
> +				mtd->erasesize_shift =  
> ffs(mtd->erasesize) - 1;
> +			else
> +				mtd->erasesize_shift = 0;
> +
> +			if (is_power_of_2(mtd->writesize))
> +				mtd->writesize_shift =  
> ffs(mtd->writesize) - 1;
> +			else
> +				mtd->writesize_shift = 0;
> +
> +			mtd->erasesize_mask = (1 <<  
> mtd->erasesize_shift) - 1;
> +			mtd->writesize_mask = (1 <<  
> mtd->writesize_shift) - 1;
> +			if ((mtd->flags & MTD_WRITEABLE) && (mtd->flags  
> & MTD_POWERUP_LOCK)) {
> +				int error;
> +				error = mtd_unlock(mtd, 0, mtd->size);
> +				if (error && error != -EOPNOTSUPP)
> +					printk(KERN_WARNING
> +			       			"%s: unlock failed,  
> writes may not work\n",
> +			       			mtd->name);
> +			}
>  			return 0;
>  		}
> 

Please factor this out into its own function that can look more like
Linux's add_mtd_device(), and have U-Boot's add_mtd_device() just be a
wrapper that does the loop and calls the internal function when it finds
a slot.

> +int mtd_device_parse_register(struct mtd_info *mtd, const char  
> **types,
> +			      struct mtd_part_parser_data *parser_data,
> +			      const struct mtd_partition *parts,
> +			      int nr_parts)
> +{
> +	int err;
> +#ifdef CONFIG_MTD_PARTITIONS
> +	struct mtd_partition *real_parts;
> +
> +	err = parse_mtd_partitions(mtd, types, &real_parts,  
> parser_data);
> +	if (err <= 0 && nr_parts && parts) {
> +		real_parts = kmemdup(parts, sizeof(*parts) * nr_parts,
> +				     GFP_KERNEL);
> +		if (!real_parts)
> +			err = -ENOMEM;
> +		else
> +			err = nr_parts;
> +	}
> +
> +	if (err > 0) {
> +		err = add_mtd_partitions(mtd, real_parts, err);
> +		kfree(real_parts);
> +	} else if (err == 0) {
> +		err = add_mtd_device(mtd);
> +		if (err == 1)
> +			err = -ENODEV;
> +	}
> +#else
> +	err = add_mtd_device(mtd);
> +	if (err == 1)
> +		err = -ENODEV;
> +#endif

I don't see parse_mtd_partitions() in U-Boot, before or after this  
patch.

I think you're doing more than updating the version, and are bringing in
functionality that U-Boot previously excluded.

> +/*
> + * This stuff for eXecute-In-Place. phys is optional and may be set  
> to NULL.
> + */
> +int mtd_point(struct mtd_info *mtd, loff_t from, size_t len, size_t  
> *retlen,
> +	      void **virt, resource_size_t *phys)

Why do we need this in U-Boot?

> +{
> +	*retlen = 0;
> +	*virt = NULL;
> +	if (phys)
> +		*phys = 0;
> +	if (!mtd->_point)
> +		return -EOPNOTSUPP;
> +	if (from < 0 || from > mtd->size || len > mtd->size - from)
> +		return -EINVAL;
> +	if (!len)
> +		return 0;
> +	return mtd->_point(mtd, from, len, retlen, virt, phys);
> +}
> +/* We probably shouldn't allow XIP if the unpoint isn't a NULL */
> +int mtd_unpoint(struct mtd_info *mtd, loff_t from, size_t len)

Please leave a blank line between functions.  It doesn't look like this
in Linux...

> +/*
> + * Allow NOMMU mmap() to directly map the device (if not NULL)
> + * - return the address to which the offset maps
> + * - return -ENOSYS to indicate refusal to do the mapping
> + */
> +unsigned long mtd_get_unmapped_area(struct mtd_info *mtd, unsigned  
> long len,
> +				    unsigned long offset, unsigned long  
> flags)
> +{
> +	if (!mtd->_get_unmapped_area)
> +		return -EOPNOTSUPP;
> +	if (offset > mtd->size || len > mtd->size - offset)
> +		return -EINVAL;
> +	return mtd->_get_unmapped_area(mtd, len, offset, flags);
> +}

More stuff that doesn't seem to belong in U-Boot.

> @@ -123,14 +120,13 @@ static int check_offs_len(struct mtd_info *mtd,
> 
>  	/* Start address must align on block boundary */
>  	if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
> -		MTDDEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n",  
> __func__);
> +		MTDDEBUG(MTD_DEBUG_LEVEL0, "%s: unaligned address\n",  
> __func__);
>  		ret = -EINVAL;
>  	}
> 
>  	/* Length must align on block boundary */
>  	if (len & ((1 << chip->phys_erase_shift) - 1)) {
> -		MTDDEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block  
> aligned\n",
> -					__func__);
> +		MTDDEBUG(MTD_DEBUG_LEVEL0, "%s: length not block  
> aligned\n", __func__);
>  		ret = -EINVAL;
>  	}

It is line-wrapped in U-Boot but not Linux, because it exceeds 80
characters in U-Boot but not Linux.  This is because
"MTD_DEBUG(MTD_DEBUG_LEVEL0" is longer than "pr_debug".

> @@ -141,14 +137,15 @@ static int check_offs_len(struct mtd_info *mtd,
>  		ret = -EINVAL;
>  	}
> 
> +
>  	return ret;
>  }

Why?  Linux doesn't look like this.

> @@ -160,22 +157,22 @@ static void nand_release_device(struct mtd_info  
> *mtd)
> 
>  /**
>   * nand_read_byte - [DEFAULT] read one byte from the chip
> - * @mtd:	MTD device structure
> + * @mtd: MTD device structure
>   *
> - * Default read function for 8bit buswith
> + * Default read function for 8bit buswidth
>   */
> -uint8_t nand_read_byte(struct mtd_info *mtd)
> +static uint8_t nand_read_byte(struct mtd_info *mtd)

NACK, this change was made deliberately for U-Boot, so that SPL can  
share
the function.

Please don't just copy over Linux's file and try to make it work.
Generate a diff of what changed in Linux since the last synchronization
point (which was 3.0, not 2.6.30) and try to apply that diff.  If U-Boot
was already different from 3.0, consider whether it was for a good
reason.  Note that the corresponding Linux commit is in the changelog  
for
the commits that last updated U-Boot's NAND code (and should be in any
future syncs).

> @@ -479,11 +468,6 @@ static int nand_block_checkbad(struct mtd_info  
> *mtd, loff_t ofs, int getchip,
>  {
>  	struct nand_chip *chip = mtd->priv;
> 
> -	if (!(chip->options & NAND_BBT_SCANNED)) {
> -		chip->options |= NAND_BBT_SCANNED;
> -		chip->scan_bbt(mtd);
> -	}
> -

Ahem:

commit fb49454b1b6c7c6e238ac3c0b1e302e73eb1a1ea
Author: Scott Wood <scottwood at freescale.com>
Date:   Mon Feb 20 14:50:39 2012 -0600

     nand: reinstate lazy bad block scanning

     commit 2a8e0fc8b3dc31a3c571e439fbf04b882c8986be ("nand: Merge  
changes
     from Linux nand driver") accidentally reverted commit
     13f0fd94e3cae6f8a0d9fba5d367e311edc8ebde ("NAND: Scan bad blocks
     lazily.").

     Reinstate the change, as amended by commit
     ff49ea8977b56916edd5b1766d9939010e30b181 ("NAND: Mark the BBT as  
scanned
     prior to calling scan_bbt.").

     Signed-off-by: Scott Wood <scottwood at freescale.com>


> @@ -144,9 +144,6 @@ int nand_torture(nand_info_t *nand, loff_t  
> offset);
>  #define NAND_LOCK_STATUS_TIGHT	0x01
>  #define NAND_LOCK_STATUS_UNLOCK 0x04
> 
> -int nand_lock(nand_info_t *meminfo, int tight);
> -int nand_unlock(nand_info_t *meminfo, loff_t start, size_t length,
> -	int allexcept);

You're changing the function signature here, but I don't see where you
update the callers in common/cmd_nand.c.

-Scott