[U-Boot] [PATCH][RFC] MTD resync from Linux-3.8-rc2+ (master)
Sergey Lapin
slapin at ossfans.org
Thu Jan 3 22:29:49 CET 2013
Dear Scott,
Thanks a lot for your review.
You made a lot of things cleaner for me, so now I better understand
what have to be done, but I need some more cleanups to get some
things more clear.
On Wed, Jan 02, 2013 at 08:53:13PM -0600, Scott Wood wrote:
> Subject line should be something like
> "mtd: sync NAND and mtdcore with Linux v3.8-rc2".
>
> What is in v3.8-rc2 that you need? I agree with others that we should
> sync from stable versions.
There are a few changes.
Mandatory ones - I need bigger buffers for OOB, this is
post 3.7 change. More than that, absence of this change makes
me unable to test the patch at all.
There is change to deselect chipselect after many operations -
which is also very important to me, but not mandatory.
Some changes I like - are pure cosmetics and can be omitted entirely
and reviewable by single person. See for yourslef:
commit d4d4f1bf6a343b25220fdcdf559fd593dd3e25a7
Author: Brian Norris <computersforpeace at gmail.com>
Date: Wed Nov 14 21:54:20 2012 -0800
mtd: nand: typo in nand_id_has_period() comments
commit ff3206b2450499203532af2505a7f6f8413e92c0
Author: Matthieu CASTET <matthieu.castet at parrot.com>
Date: Tue Nov 6 11:51:43 2012 +0100
mtd: nand: onfi need to be probed in 8 bits mode
commit 64b37b2a63eb2f80b65c7185f0013f8ffc637ae3
Author: Matthieu CASTET <matthieu.castet at parrot.com>
Date: Tue Nov 6 11:51:44 2012 +0100
mtd: nand: add NAND_BUSWIDTH_AUTO to autodetect bus width
commit 2fd71a294a0aac407ec69e04916dc28eb39c8ac0
Author: Matthieu CASTET <matthieu.castet at parrot.com>
Date: Thu Nov 22 18:33:40 2012 +0100
mtd: nand: print flash size during detection
commit ca6a24893073414c549920d93bac57d46d7f3e1e
Author: Matthieu CASTET <matthieu.castet at parrot.com>
Date: Thu Nov 22 18:31:28 2012 +0100
mted: nand_wait_ready timeout fix
commit 6a8214aa3d323d2e185523ea112116759bc3c5ce
Author: Huang Shijie <b32955 at freescale.com>
Date: Mon Nov 19 14:43:30 2012 +0800
mtd: remove the "chip" parameter in nand_get_device()
ommit b0bb6903c8fca2d5ebef1f8ae63d420eb931bb1e
Author: Huang Shijie <b32955 at freescale.com>
Date: Mon Nov 19 14:43:29 2012 +0800
mtd: remove the de-select chip code in nand_release_device()
commit 064a7694b5347208febeb7aba7c90d38934ec8a1
Author: Masanari Iida <standby24x7 at gmail.com>
Date: Fri Nov 9 23:20:58 2012 +0900
mtd: Fix typo mtd/tests
commit 07300164657526d8d26c626c43723c310fbf3616
Author: Huang Shijie <b32955 at freescale.com>
Date: Fri Nov 9 16:23:45 2012 +0800
mtd: de-select the chip when it is not used
commit f251b8dfdd0721255ea11751cdc282834e43b74e
Author: Matthieu CASTET <matthieu.castet at parrot.com>
Date: Mon Nov 5 15:00:44 2012 +0100
mtd: nand_wait: warn if the nand is busy on exit
commit 9ef525a9141b14d23613faad303cf48a20814f1b
Author: Robert P. J. Day <rpjday at crashcourse.ca>
Date: Thu Oct 25 09:43:10 2012 -0400
mtd: Fix kernel-doc content to avoid warning.
commit 7483096665161d2567c2717e001654ad653d944e
Author: Huang Shijie <shijie8 at gmail.com>
Date: Sun Oct 14 23:47:24 2012 -0400
mtd: use the NAND_STATUS_FAIL to replace the hardcode
commit 2f25ae97fe4b424d88d765797c46456c7c0f1bae
Author: Vipin Kumar <vipin.kumar at st.com>
Date: Tue Oct 9 16:14:53 2012 +0530
mtd: nand: Increase the ecc placement locations to 640
slapin> I depend on this change.
Other changes, as you can see, are minor, easily reviewable,
and can be managed by single person.
>
> On 12/28/2012 05:19:17 AM, Sergey Lapin wrote:
> >
> >Signed-off-by: Sergey Lapin <slapin at ossfans.org>
> >---
> >
> >Warning: no testing, except for single board,
> >was made with this patch.
> >
> >It is just basic resync for NAND parts of MTD with some
> >modifications to mtdcore
> >I tried to be as close to original as possible.
>
> What is the code size change with this patch?
Not calculated yet, will do. Any tools doing that or simple du -sk u-boot.bin
before and after patch will do?
>
> >diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> >index 543c845..ba1e22a 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)))
> >+COBJS-y += mtdcore.o
> >+endif
>
> So NAND now depends on mtdcore... what changed to make this necessary?
I've added there function wrappers like in kernel, which run hooks
like mtd->read_oob(mtd...) becomes mtd_read_page(mtd...),
which looks more readable... I have not applied the rest of mtdcore changes,
as u-boot's mtdcore is quite different from kernel's.
>
> And shouldn't we fix board files to include both, rather than hack
> up the makefiles in this one spot?
Probably, or we can put the hooks to separate file, and make them
mtdcore-independant. I just fillowed kernel's path there.
>
> What about the #ifdef CONFIG_MTD_DEVICE in drivers/mtd/nand/nand.c?
>
> >diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> >index 3a81ada..1e722a1 100644
> >--- a/drivers/mtd/mtdcore.c
> >+++ b/drivers/mtd/mtdcore.c
> >@@ -9,6 +9,7 @@
> >
> > #include <linux/mtd/mtd.h>
> > #include <linux/compat.h>
> >+#include <linux/mtd/partitions.h>
> > #include <ubi_uboot.h>
> >
> > struct mtd_info *mtd_table[MAX_MTD_DEVICES];
> >@@ -32,6 +33,30 @@ int add_mtd_device(struct mtd_info *mtd)
> > our caller is still holding us here. So none
> > of this try_ nonsense, and no bitching about it
> > either. :) */
> >+ /* default value if not set by driver */
> >+ if (mtd->bitflip_threshold == 0)
> >+ mtd->bitflip_threshold = mtd->ecc_strength;
> >+
> >+ if (is_power_of_2(mtd->erasesize))
> >+ mtd->erasesize_shift = ffs(mtd->erasesize) - 1;
> >+ else
> >+ mtd->erasesize_shift = 0;
> >+
> >+ if (is_power_of_2(mtd->writesize))
> >+ mtd->writesize_shift = ffs(mtd->writesize) - 1;
> >+ else
> >+ mtd->writesize_shift = 0;
> >+
> >+ mtd->erasesize_mask = (1 << mtd->erasesize_shift) - 1;
> >+ mtd->writesize_mask = (1 << mtd->writesize_shift) - 1;
> >+ if ((mtd->flags & MTD_WRITEABLE) && (mtd->flags &
> >MTD_POWERUP_LOCK)) {
> >+ int error;
> >+ error = mtd_unlock(mtd, 0, mtd->size);
> >+ if (error && error != -EOPNOTSUPP)
> >+ printk(KERN_WARNING
> >+ "%s: unlock failed, writes may not work\n",
> >+ mtd->name);
> >+ }
> > return 0;
> > }
> >
>
> Please factor this out into its own function that can look more like
> Linux's add_mtd_device(), and have U-Boot's add_mtd_device() just be a
> wrapper that does the loop and calls the internal function when it finds
> a slot.
Please, be more specific about this. Which loop? Or you don't like the function size?
>
> >+int mtd_device_parse_register(struct mtd_info *mtd, const char
> >**types,
> >+ struct mtd_part_parser_data *parser_data,
> >+ const struct mtd_partition *parts,
> >+ int nr_parts)
> >+{
> >+ int err;
> >+#ifdef CONFIG_MTD_PARTITIONS
> >+ struct mtd_partition *real_parts;
> >+
> >+ err = parse_mtd_partitions(mtd, types, &real_parts,
> >parser_data);
> >+ if (err <= 0 && nr_parts && parts) {
> >+ real_parts = kmemdup(parts, sizeof(*parts) * nr_parts,
> >+ GFP_KERNEL);
> >+ if (!real_parts)
> >+ err = -ENOMEM;
> >+ else
> >+ err = nr_parts;
> >+ }
> >+
> >+ if (err > 0) {
> >+ err = add_mtd_partitions(mtd, real_parts, err);
> >+ kfree(real_parts);
> >+ } else if (err == 0) {
> >+ err = add_mtd_device(mtd);
> >+ if (err == 1)
> >+ err = -ENODEV;
> >+ }
> >+#else
> >+ err = add_mtd_device(mtd);
> >+ if (err == 1)
> >+ err = -ENODEV;
> >+#endif
>
> I don't see parse_mtd_partitions() in U-Boot, before or after this
> patch.
>
> I think you're doing more than updating the version, and are bringing in
> functionality that U-Boot previously excluded.
OK, will remove. IIRC, I've seen a few of #ifdef CONFIG_MTD_PARTITIONS
in u-boot though.
>
> >+/*
> >+ * This stuff for eXecute-In-Place. phys is optional and may be
> >set to NULL.
> >+ */
> >+int mtd_point(struct mtd_info *mtd, loff_t from, size_t len,
> >size_t *retlen,
> >+ void **virt, resource_size_t *phys)
>
> Why do we need this in U-Boot?
Just tried to be complete there. Will remove.
>
> >+{
> >+ *retlen = 0;
> >+ *virt = NULL;
> >+ if (phys)
> >+ *phys = 0;
> >+ if (!mtd->_point)
> >+ return -EOPNOTSUPP;
> >+ if (from < 0 || from > mtd->size || len > mtd->size - from)
> >+ return -EINVAL;
> >+ if (!len)
> >+ return 0;
> >+ return mtd->_point(mtd, from, len, retlen, virt, phys);
> >+}
> >+/* We probably shouldn't allow XIP if the unpoint isn't a NULL */
> >+int mtd_unpoint(struct mtd_info *mtd, loff_t from, size_t len)
>
> Please leave a blank line between functions. It doesn't look like this
> in Linux...
OK
>
> >+/*
> >+ * Allow NOMMU mmap() to directly map the device (if not NULL)
> >+ * - return the address to which the offset maps
> >+ * - return -ENOSYS to indicate refusal to do the mapping
> >+ */
> >+unsigned long mtd_get_unmapped_area(struct mtd_info *mtd,
> >unsigned long len,
> >+ unsigned long offset, unsigned long flags)
> >+{
> >+ if (!mtd->_get_unmapped_area)
> >+ return -EOPNOTSUPP;
> >+ if (offset > mtd->size || len > mtd->size - offset)
> >+ return -EINVAL;
> >+ return mtd->_get_unmapped_area(mtd, len, offset, flags);
> >+}
>
> More stuff that doesn't seem to belong in U-Boot.
Will remove.
>
> >@@ -123,14 +120,13 @@ static int check_offs_len(struct mtd_info *mtd,
> >
> > /* Start address must align on block boundary */
> > if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
> >- MTDDEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n",
> >__func__);
> >+ MTDDEBUG(MTD_DEBUG_LEVEL0, "%s: unaligned address\n",
> >__func__);
> > ret = -EINVAL;
> > }
> >
> > /* Length must align on block boundary */
> > if (len & ((1 << chip->phys_erase_shift) - 1)) {
> >- MTDDEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
> >- __func__);
> >+ MTDDEBUG(MTD_DEBUG_LEVEL0, "%s: length not block aligned\n",
> >__func__);
> > ret = -EINVAL;
> > }
>
> It is line-wrapped in U-Boot but not Linux, because it exceeds 80
> characters in U-Boot but not Linux. This is because
> "MTD_DEBUG(MTD_DEBUG_LEVEL0" is longer than "pr_debug".
I don't like this approach, too, but I don't want to diverge from Linux too much.
Can I just put these pr_* macros where they are and implement them using
MTDDEBUG(.. macro in some header, or should I replace them?
There is some WARN_ON things, too. But these are already implemented in u-boot.
Or I can leave things as they are and just fix all checkpatch.pl warnings.
>
> >@@ -141,14 +137,15 @@ static int check_offs_len(struct mtd_info *mtd,
> > ret = -EINVAL;
> > }
> >
> >+
> > return ret;
> > }
>
> Why? Linux doesn't look like this.
Sorry, that is an effect of my ad-hoc implementations of u-boot changes to nand_base.c
functions. Will fix that.
>
> >@@ -160,22 +157,22 @@ static void nand_release_device(struct
> >mtd_info *mtd)
> >
> > /**
> > * nand_read_byte - [DEFAULT] read one byte from the chip
> >- * @mtd: MTD device structure
> >+ * @mtd: MTD device structure
> > *
> >- * Default read function for 8bit buswith
> >+ * Default read function for 8bit buswidth
> > */
> >-uint8_t nand_read_byte(struct mtd_info *mtd)
> >+static uint8_t nand_read_byte(struct mtd_info *mtd)
>
> NACK, this change was made deliberately for U-Boot, so that SPL can
> share
> the function.
Will fix.
>
> Please don't just copy over Linux's file and try to make it work.
> Generate a diff of what changed in Linux since the last synchronization
> point (which was 3.0, not 2.6.30) and try to apply that diff. If U-Boot
> was already different from 3.0, consider whether it was for a good
> reason. Note that the corresponding Linux commit is in the
> changelog for
> the commits that last updated U-Boot's NAND code (and should be in any
> future syncs).
Could you please point me on that commit (where last Linux MTD commit is),
as I seems to be unable to find it.
>
> >@@ -479,11 +468,6 @@ static int nand_block_checkbad(struct
> >mtd_info *mtd, loff_t ofs, int getchip,
> > {
> > struct nand_chip *chip = mtd->priv;
> >
> >- if (!(chip->options & NAND_BBT_SCANNED)) {
> >- chip->options |= NAND_BBT_SCANNED;
> >- chip->scan_bbt(mtd);
> >- }
> >-
>
> Ahem:
>
> commit fb49454b1b6c7c6e238ac3c0b1e302e73eb1a1ea
> Author: Scott Wood <scottwood at freescale.com>
> Date: Mon Feb 20 14:50:39 2012 -0600
>
> nand: reinstate lazy bad block scanning
>
> commit 2a8e0fc8b3dc31a3c571e439fbf04b882c8986be ("nand: Merge
> changes
> from Linux nand driver") accidentally reverted commit
> 13f0fd94e3cae6f8a0d9fba5d367e311edc8ebde ("NAND: Scan bad blocks
> lazily.").
>
> Reinstate the change, as amended by commit
> ff49ea8977b56916edd5b1766d9939010e30b181 ("NAND: Mark the BBT as
> scanned
> prior to calling scan_bbt.").
>
> Signed-off-by: Scott Wood <scottwood at freescale.com>
Thanks, will fix that.
>
>
> >@@ -144,9 +144,6 @@ int nand_torture(nand_info_t *nand, loff_t
> >offset);
> > #define NAND_LOCK_STATUS_TIGHT 0x01
> > #define NAND_LOCK_STATUS_UNLOCK 0x04
> >
> >-int nand_lock(nand_info_t *meminfo, int tight);
> >-int nand_unlock(nand_info_t *meminfo, loff_t start, size_t length,
> >- int allexcept);
>
> You're changing the function signature here, but I don't see where you
> update the callers in common/cmd_nand.c.
This function is called via wrapper.
cmd_nand is changed to call mtd_* functions, not mtd-> callbacks.
So, to summarize - the policy is to remove code, which is not used in u-boot,
from nand_base.c, am I right?
S.
More information about the U-Boot
mailing list