[U-Boot] [PATCH v2] mtd: resync with Linux-3.7.1

[Scott Wood]

On 01/14/2013 07:46:50 AM, Sergey Lapin wrote:
> This patch is essentially an update of u-boot MTD subsystem to
> the state of Linux-3.7.1 with exclusion of some bits:
> 
> - the update is concentrated on NAND, no onenand or CFI/NOR/SPI
> flashes interfaces are updated EXCEPT for API changes.
> 
> - new large NAND chips support is there, though some updates
> have got in Linux-3.8.-rc1, (which will follow on top of this patch).
> 
> To produce this update I used tag v3.7.1 of linux-stable repository.
> 
> The update was made using application of relevant patches,
> with changes relevant to U-Boot-only stuff sticked together
> to keep bisectability. Then all changes were grouped together
> to this patch.
> 
> Signed-off-by: Sergey Lapin <slapin at ossfans.org>
> ---

Applying: mtd: resync with Linux-3.7.1
/home/scott/fsl/git/u-boot/upstream/.git/rebase-apply/patch:4292: space  
before tab in indent.
  		chip->ecc.strength =
/home/scott/fsl/git/u-boot/upstream/.git/rebase-apply/patch:835: new  
blank line at EOF.
+
/home/scott/fsl/git/u-boot/upstream/.git/rebase-apply/patch:6011: new  
blank line at EOF.
+
/home/scott/fsl/git/u-boot/upstream/.git/rebase-apply/patch:7970: new  
blank line at EOF.
+
warning: 4 lines add whitespace errors.

Are these whitespace errors in Linux?

I tried booting on P2020RDB-PC_NAND, and got this:

> L2:    512 KB already enabled, moving to 0xf8f80000
> NAND:  BUG: failure at nand_base.c:3214/nand_scan_tail()!
> BUG!
> ### ERROR ### Please RESET the board ###

It boots fine without this patch.  nand_base.c:3214 is complaining about
missing ecc.strength.  You need to update existing U-Boot drivers for  
new
API so that nothing breaks.  Ask maintainers of particular drivers for
help if necessary.

You'll probably need to go through the various NAND patches between 3.0
and 3.7.1 looking for API changes, and make sure that they're all
accounted for, beyond just making things build.

> diff --git a/board/ait/cam_enc_4xx/cam_enc_4xx.c  
> b/board/ait/cam_enc_4xx/cam_enc_4xx.c
> index 32b28f9..2a0c31c 100644
> --- a/board/ait/cam_enc_4xx/cam_enc_4xx.c
> +++ b/board/ait/cam_enc_4xx/cam_enc_4xx.c
> @@ -120,7 +120,7 @@ int board_eth_init(bd_t *bis)
>  #ifdef CONFIG_NAND_DAVINCI
>  static int
>  davinci_std_read_page_syndrome(struct mtd_info *mtd, struct  
> nand_chip *chip,
> -				   uint8_t *buf, int page)
> +				   uint8_t *buf, int oob_required, int  
> page)
>  {
>  	struct nand_chip *this = mtd->priv;
>  	int i, eccsize = chip->ecc.size;
> @@ -167,8 +167,9 @@ davinci_std_read_page_syndrome(struct mtd_info  
> *mtd, struct nand_chip *chip,
>  	return 0;
>  }

We really should not be having NAND driver code (stuff that interacts
with the NAND API; not hardware setup) outside of drivers/mtd/nand.

> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 543c845..99f39fc 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)$(CONFIG_CMD_ONENAND)))
> +COBJS-y += mtdcore.o
> +endif

Please just require users of CONFIG_CMD_NAND or CONFIG_CMD_ONENAND to
also select CONFIG_MTD_DEVICE, if it's not going to be practical to do
without it -- and remove the "#ifdef CONFIG_MTD_DEVICE" from nand.c.

Could you explain why it's no longer practical to have NAND by itself?

> diff --git a/drivers/mtd/nand/diskonchip.c  
> b/drivers/mtd/nand/diskonchip.c
> index edf3a09..104d97f 100644
> --- a/drivers/mtd/nand/diskonchip.c
> +++ b/drivers/mtd/nand/diskonchip.c
> @@ -134,7 +134,7 @@ static struct rs_control *rs_decoder;
> 
>  /*
>   * The HW decoder in the DoC ASIC's provides us a error syndrome,
> - * which we must convert to a standard syndrom usable by the generic
> + * which we must convert to a standard syndrome usable by the generic
>   * Reed-Solomon library code.
>   *
>   * Fabrice Bellard figured this out in the old docecc code. I added

This file should just go away, as nobody has stepped up to fix and use
it.

>  #ifdef CONFIG_MTD_DEBUG
> +#define pr_debug(args...)	MTDDEBUG(MTD_DEBUG_LEVEL0, args)
>  #define MTDDEBUG(n, args...)				\
>  	do {						\
>  		if (n <= CONFIG_MTD_DEBUG_VERBOSE)	\
>  			printk(KERN_INFO args);		\
>  	} while(0)
>  #else /* CONFIG_MTD_DEBUG */
> +#define pr_debug(args...)
>  #define MTDDEBUG(n, args...)				\
>  	do {						\
>  		if (0)					\
>  			printk(KERN_INFO args);		\
>  	} while(0)
>  #endif /* CONFIG_MTD_DEBUG */

If you define pr_debug() to be absolutely nothing, you won't catch  
errors
until CONFIG_MTD_DEBUG is actually turned on.  That's why MTDDEBUG does
the "if (0)" thing.

pr_debug() should not be defined in terms of CONFIG_MTD_DEBUG.  What if
this header gets included by some other part of U-Boot?  What if some
other part of U-Boot also wants pr_debug, but based on a different
subsystem's CONFIG_FOO_DEBUG?

> +#define pr_info(args...)	MTDDEBUG(MTD_DEBUG_LEVEL0, args)
> +#define pr_warn(args...)	MTDDEBUG(MTD_DEBUG_LEVEL0, args)
> +#define pr_err(args...)		MTDDEBUG(MTD_DEBUG_LEVEL0, args)

These should be ordinary printf, not MTDDEBUG.  Under normal
(non-debug) circumstances MTDDEBUG is a no-op.  We want to see errors  
and
warnings always.

Plus, these should be defined somewhere that isn't MTD-specific.

> +static inline int mtd_is_bitflip(int err) {
> +	return err == -EUCLEAN;
> +}
> +
> +static inline int mtd_is_eccerr(int err) {
> +	return err == -EBADMSG;
> +}
> +
> +static inline int mtd_is_bitflip_or_eccerr(int err) {
> +	return mtd_is_bitflip(err) || mtd_is_eccerr(err);
> +}

Sigh, Linux isn't following its own coding style.

> diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
> new file mode 100644
> index 0000000..d51c1ab
> --- /dev/null
> +++ b/include/mtd/mtd-abi.h
> @@ -0,0 +1,209 @@
> +/*
> + * $Id: mtd-abi.h,v 1.13 2005/11/07 11:14:56 gleixner Exp $
> + *
> + * Portions of MTD ABI definition which are shared by kernel and  
> user space
> + */
> +
> +#ifndef __MTD_ABI_H__
> +#define __MTD_ABI_H__
> +
> +#if 1
> +#include <linux/compat.h>
> +#endif

Why "#if 1"?

-Scott