On 01/17/2013 06:07:39 AM, Sergey Lapin wrote:
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?
They don't appear to have come from the Linux sources, so yes, please
fix them.
> 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.
OK... ecc.strength is going to need to be fixed on several drivers
(pretty much everything that uses hardware ecc). When I fixed that,
elbc worked.
> >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.
Yes, it's not a problem with this patch, just something that this patch
made me notice (hence the CC to Heiko).
> >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
I'd say move them to another file in drivers/mtd. mtdapi.c?
> >+#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?
Put them in include/linux/compat.h
So, what is next step?
Once ecc.strength and the other review issues are dealt with, it should
be good to go in.
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
