| Message ID | 1268772081-6534-1-git-send-email-khilman@deeprootsystems.com |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2010-03-16 at 13:41 -0700, Kevin Hilman wrote: > From: Sudhakar Rajashekhara <sudhakar....@ti.com> > > This patch implements memory accessor interface in the MTD > layer which enables the kernel to access flash data. > > This patch adds two new members to the mtd_partition structure, > a function handler which will be called during setup of the > partition and an argument to be passed to this setup function. I've pushed this one to my l2-mtd-2.6.git tree, branch "dunno".
On Tue, 2010-03-16 at 13:41 -0700, Kevin Hilman wrote: > From: Sudhakar Rajashekhara <sudhakar....@ti.com> > > This patch implements memory accessor interface in the MTD > layer which enables the kernel to access flash data. > > This patch adds two new members to the mtd_partition structure, > a function handler which will be called during setup of the > partition and an argument to be passed to this setup function. Ick. I don't mind providing the mtd_macc_{read,write} helper functions, although they should work on generic MTD devices not just on partitions. But do we really have to do the callout to arbitrary functions from _within_ the core MTD code... why can't we do that in the board driver?
Hi David, On Fri, May 14, 2010 at 05:20:29, David Woodhouse wrote: > On Tue, 2010-03-16 at 13:41 -0700, Kevin Hilman wrote: > > From: Sudhakar Rajashekhara <sudhakar....@ti.com> > > > > This patch implements memory accessor interface in the MTD > > layer which enables the kernel to access flash data. > > > > This patch adds two new members to the mtd_partition structure, > > a function handler which will be called during setup of the > > partition and an argument to be passed to this setup function. > > Ick. > > I don't mind providing the mtd_macc_{read,write} helper functions, > although they should work on generic MTD devices not just on partitions. > But do we really have to do the callout to arbitrary functions from > _within_ the core MTD code... why can't we do that in the board driver? > First of all apologies for replying to this thread *very* late. This thread started in response to the patch at http://lkml.org/lkml/2010/3/16/380. I think you are OK with the mtd_macc_{read,write} functions being provided within the MTD core but you are objecting to the extra ->setup method in struct mtd_partition. Without adding the setup method in mtd_parition structure, the MTD memory accessor interface users will not knowing when to call the mtd_macc_{read,write} functions. These functions should be called after the MTD partitions are set up. If the ->setup method is moved to the board driver, I'll get into the initialization sequence problem, which I want to avoid. Thanks, Sudhakar
> > > This patch adds two new members to the > mtd_partition structure, > > > a function handler which will be > > > called during setup of the > > > partition and an argument to be passed to this > setup function. > > > > Ick. > > > > I don't mind providing the mtd_macc_{read,write} > helper functions, > > although they should work on generic MTD > > >devices not just on partitions. > > But do we really have to do the callout to > > arbitrary functions from > > _within_ the core MTD code... why can't we > > do that in the board driver? I think the short answer is that the callout is what provides the board drivers enough information to make the correct calls.
Hi Dave, On Wed, Jul 07, 2010 at 16:38:54, David Brownell wrote: > > > > > This patch adds two new members to the > > mtd_partition structure, > > > > a function handler which will be > > > > called during setup of the > > > > partition and an argument to be passed to this > > setup function. > > > > > > Ick. > > > > > > I don't mind providing the mtd_macc_{read,write} > > helper functions, > > > although they should work on generic MTD > > > >devices not just on partitions. > > > But do we really have to do the callout to > > > arbitrary functions from > > > _within_ the core MTD code... why can't we > > > do that in the board driver? > > I think the short answer is that the callout is > what provides the board drivers enough information > to make the correct calls. > If you agree to what the patch is doing, then can you Ack it? Thanks, Sudhakar
> > I think the short answer is that the callout is > > what provides the board drivers enough information > > to make the correct calls. > > > > If you agree to what the patch is doing, then can you Ack it? Iff it's resent; it's not in my mbox any more.
On Wed, 2010-07-07 at 04:08 -0700, David Brownell wrote: > > I think the short answer is that the callout is > what provides the board drivers enough information > to make the correct calls. I don't see how. The only information it passes to the callout is the information it was already *given* in the partition structure. I'm more inclined to believe Sudhakar's claim that you'll get an 'initialization sequence problem', although I'm not sure I believe it can't be solved in a better way than this. I'm also unhappy that it only works on partitioned devices -- that seems wrong.
--- On Wed, 8/4/10, David Woodhouse <dwmw2@infradead.org> wrote: > On Wed, 2010-07-07 at 04:08 -0700, > David Brownell wrote: > > > > I think the short answer is that the callout is > > what provides the board drivers enough information > > to make the correct calls. EMPHASIS ON "CORRECT" ... > I don't see how. > > The only information it passes to the callout is the > information it was > already *given* in the partition structure. There can be multiple such structures, each of which describes different data to be extracted from different persistent media contexts. Examples: one context holds one MAC address (and another, a different one) ... another might embed calibration data; another, serial numbers; etc. Pass the wrong context around, you've trashed all the data instead of getting it right. > I'm more inclined to believe Sudhakar's claim that you'll > get an > 'initialization sequence problem', although I'm not sure I > believe it > can't be solved in a better way than this. "Initialization sequence" is a grab-bag category that covers most init issues. Point is to ensure that enough of the right context information is available to initialize correctly. So the right data is extracted and passed on. > > I'm also unhappy that it only works on partitioned devices > -- that seems wrong. Very different issue. Seems easily fixable if needed. Agreed that e.g. EEPROMS won't often be partitioned (unlike flash).
On Wed, 2010-08-04 at 03:31 -0700, David Brownell wrote: > Point is to ensure that enough of the right context > information is available to initialize correctly. > So the right data is extracted and passed on. Forgive me if I'm being dim (and in particular, please forgive me if I'm going over something that was already discussed; I know it's been a while). But I don't see why it needs to be passed through the core MTD code. To take the simple case of an unpartitioned MTD device -- why can't the map driver (or whatever) just call the maccessor setup function for itself, directly, right after calling add_mtd_device() with its newly-probed MTD device? And for partitions, why can't it do the same, on the appropriate partition. OK, the answer to the latter question is that you don't actually *have* the pointers to each partition you register. But that's easily fixed. If we make add_mtd_partitions() take an extra 'struct mtd_info **' argument and put pointers to the slave mtd 'devices' into that, it means that your board driver *can* reliably get the mtd pointer for the fourth partition, or whatever it needs. And can then just do the memory accessor setup for itself. Isn't that enough?
--- On Wed, 8/4/10, David Woodhouse <dwmw2@infradead.org> wrote: > > Point is to ensure that enough of the right context > > information is available to initialize correctly. > > So the right data is extracted and passed on. And also, ISTR, that the mechanism is general enough to work with both MTD and EEPROM ... > > Forgive me if I'm being dim (and in particular, please > forgive me if I'm > going over something that was already discussed; I know > it's been a > while). I also am at risk of getting lost in a pile of hypotheticals which have been left behind earlier in these threads. But I don't see why it needs to be passed through > the core MTD code. > > To take the simple case of an unpartitioned MTD device -- > why can't the > map driver (or whatever) just call the maccessor setup > function for > itself, directly, right after calling add_mtd_device() with > its newly-probed MTD device? No idea, except that doing it once rather than modifying every driver would seem healthier. Surely changing all drivers is a Bad Thing. > > And for partitions, why can't it do the same, on the > appropriate partition. > > OK, the answer to the latter question is that you don't > actually *have* > the pointers to each partition you register. But that's > easily fixed. > > If we make add_mtd_partitions() take an extra 'struct > mtd_info **' > argument and put pointers to the slave mtd 'devices' into > that, it means > that your board driver *can* reliably get the mtd pointer > for the fourth > partition, or whatever it needs. And can then just do the > memory > accessor setup for itself. > > Isn't that enough? Might be. Not my patch though... You asked why the context was needed along with the partition data (otherwise not available); I answered. Still haven't seen a better patch though.
Hi, On Wed, Aug 04, 2010 at 16:38:47, David Woodhouse wrote: > On Wed, 2010-08-04 at 03:31 -0700, David Brownell wrote: > > Point is to ensure that enough of the right context > > information is available to initialize correctly. > > So the right data is extracted and passed on. > > Forgive me if I'm being dim (and in particular, please forgive me if I'm > going over something that was already discussed; I know it's been a > while). But I don't see why it needs to be passed through the core MTD > code. > > To take the simple case of an unpartitioned MTD device -- why can't the > map driver (or whatever) just call the maccessor setup function for > itself, directly, right after calling add_mtd_device() with its > newly-probed MTD device? > > And for partitions, why can't it do the same, on the appropriate > partition. > > OK, the answer to the latter question is that you don't actually *have* > the pointers to each partition you register. But that's easily fixed. > > If we make add_mtd_partitions() take an extra 'struct mtd_info **' > argument and put pointers to the slave mtd 'devices' into that, it means > that your board driver *can* reliably get the mtd pointer for the fourth > partition, or whatever it needs. And can then just do the memory > accessor setup for itself. > > Isn't that enough? > Thanks for the feedback. I'll be re-working on this patch and will re-post the updated patch soon. Regards, Sudhakar
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index b8043a9..9f4d5f8 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -26,6 +26,7 @@ static LIST_HEAD(mtd_partitions); struct mtd_part { struct mtd_info mtd; struct mtd_info *master; + struct memory_accessor macc; uint64_t offset; struct list_head list; }; @@ -327,6 +328,39 @@ int del_mtd_partitions(struct mtd_info *master) } EXPORT_SYMBOL(del_mtd_partitions); +/* + * This lets other kernel code access the flash data. For example, it + * might hold a board's Ethernet address, or board-specific calibration + * data generated on the manufacturing floor. + */ +static ssize_t mtd_macc_read(struct memory_accessor *macc, char *buf, + off_t offset, size_t count) +{ + struct mtd_part *part = container_of(macc, struct mtd_part, macc); + ssize_t ret = -EIO; + size_t retlen; + + if (part_read((struct mtd_info *)part, offset, count, + &retlen, buf) == 0) + ret = retlen; + + return ret; +} + +static ssize_t mtd_macc_write(struct memory_accessor *macc, const char *buf, + off_t offset, size_t count) +{ + struct mtd_part *part = container_of(macc, struct mtd_part, macc); + ssize_t ret = -EIO; + size_t retlen; + + if (part_write((struct mtd_info *)part, offset, count, + &retlen, buf) == 0) + ret = retlen; + + return ret; +} + static struct mtd_part *add_one_partition(struct mtd_info *master, const struct mtd_partition *part, int partno, uint64_t cur_offset) @@ -364,6 +398,9 @@ static struct mtd_part *add_one_partition(struct mtd_info *master, slave->mtd.read = part_read; slave->mtd.write = part_write; + slave->macc.read = mtd_macc_read; + slave->macc.write = mtd_macc_write; + if (master->panic_write) slave->mtd.panic_write = part_panic_write; @@ -428,6 +465,9 @@ static struct mtd_part *add_one_partition(struct mtd_info *master, printk(KERN_NOTICE "0x%012llx-0x%012llx : \"%s\"\n", (unsigned long long)slave->offset, (unsigned long long)(slave->offset + slave->mtd.size), slave->mtd.name); + if (part->setup) + part->setup(&slave->macc, (void *)part->context); + /* let's do some sanity checks */ if (slave->offset >= master->size) { /* let's register it anyway to preserve ordering */ diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h index 274b619..39782a6 100644 --- a/include/linux/mtd/partitions.h +++ b/include/linux/mtd/partitions.h @@ -10,6 +10,7 @@ #define MTD_PARTITIONS_H #include <linux/types.h> +#include <linux/memory.h> /* @@ -40,6 +41,8 @@ struct mtd_partition { uint64_t offset; /* offset within the master MTD space */ uint32_t mask_flags; /* master MTD flags to mask out for this partition */ struct nand_ecclayout *ecclayout; /* out of band layout for this partition (NAND only)*/ + void (*setup)(struct memory_accessor *, void *context); + void *context; }; #define MTDPART_OFS_NXTBLK (-2)