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 slapin@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, intpage)
{ 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