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 <sla...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
