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

[Sergey Lapin]

Dear Scott,
thanks a lot for your review,
see below.

On Wed, Jan 16, 2013 at 04:09:45PM -0600, Scott Wood wrote:
> 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?
Yeah, this thing wonders me why. I can fix these without too much
hussle, should I?
> 
> 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 ###
Can you debug this farther, as I don't have devices with hardware ECC
NAND, and was unable to convince anyone for testing?
Or, better, direct access to some board for several days could be nice.

> 
> 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.
I updated all drivers I could test and tried to do that for all others,
but it seems it won't work without testing.

> 
> 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.
This is what I did most of time, shit happens.

> 
> >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.
This probably should be split.
> 
> >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?

I have dilemma here:

New MTD API instead of calling mtd->foo(mtd...) uses mtd_foo(mtd...)
functions. In Linux, these are defined in mtdcore.c

We can either move these functions from mtdcore.c somewhere (where?), or
#ifndef unrelated parts from mtdcore.c

> 
> >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.
So, should I add separate header?
> 
> >+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"?

Sorry, this is probably my conflict resolution issue, will fix.

So, what is next step?

Thanks a lot,
S.