parse_cmdline_partitions equivalent for map_info

parse_cmdline_partitions equivalent for map_info

[Frank Neuber]

Hi again,
I'm happy with the parse_cmdline_partitions function to pass the
partitioning informations to the kernel.
Now I'm looking for the same feature to pass the physically informations
to the kernel (struct map_info).
I could add this simply to the cmdline.c file based on the
parse_cmdline_partitions function.
But before I do this I would ask the MTD people if this is done now? 

Regards,
 Frank
-- 
Dipl.-Ing. Elektrotechnik           convergence / HW
Frank Neuber                        Rosenthalerstr.51 / 10178 Berlin
Email:  neuber@convergence.de           Phone:  +49(0)30-72 62 06 76
WWW:    www.convergence.de              Fax:    +49(0)30-72 62 06 55

Re: parse_cmdline_partitions equivalent for map_info

[David Woodhouse]

neuber@convergence.de said:
> Now I'm looking for the same feature to pass the physically
> informations to the kernel (struct map_info). I could add this simply
> to the cmdline.c file based on the parse_cmdline_partitions function.

That seems like a relatively sensible addition to physmap.c. You could make 
it take the probe type that way too...

 'physmap=0xe0000000,0x100000,16,cfi'

--
dwmw2

Re: parse_cmdline_partitions equivalent for map_info

[Jörn Engel]

On Fri, 18 October 2002 13:14:48 +0100, David Woodhouse wrote:
> neuber@convergence.de said:
> > Now I'm looking for the same feature to pass the physically
> > informations to the kernel (struct map_info). I could add this simply
> > to the cmdline.c file based on the parse_cmdline_partitions function.
> 
> That seems like a relatively sensible addition to physmap.c. You could make 
> it take the probe type that way too...
> 
>  'physmap=0xe0000000,0x100000,16,cfi'

Hmm, somewhere in my collection of yet-to-be-committed patches, I have
a variant of the physmap driver that does exactly this and is capable
of mapping up to eight independent devices. Interested?

Jörn

-- 
Write programs that do one thing and do it well. Write programs to work
together. Write programs to handle text streams, because that is a
universal interface. 
-- Doug MacIlroy

Re: parse_cmdline_partitions equivalent for map_info

[Frank Neuber]

On Sat, Oct 19, 2002 at 06:44:45PM +0200, Jörn Engel wrote:
> On Fri, 18 October 2002 13:14:48 +0100, David Woodhouse wrote:
> > neuber@convergence.de said:
> > > Now I'm looking for the same feature to pass the physically
> > > informations to the kernel (struct map_info). I could add this simply
> > > to the cmdline.c file based on the parse_cmdline_partitions function.
> > 
> > That seems like a relatively sensible addition to physmap.c. You could make 
> > it take the probe type that way too...
> > 
> >  'physmap=0xe0000000,0x100000,16,cfi'
> 
> Hmm, somewhere in my collection of yet-to-be-committed patches, I have
> a variant of the physmap driver that does exactly this and is capable
> of mapping up to eight independent devices. Interested?
> 
Yes, it would be nice to test your code :-)
Maybe we can do this more generic to add this to cmdline.c.
What do you think?
 Frank

-- 
Dipl.-Ing. Elektrotechnik           convergence / HW
Frank Neuber                        Rosenthalerstr.51 / 10178 Berlin
Email:  neuber@convergence.de           Phone:  +49(0)30-72 62 06 76
WWW:    www.convergence.de              Fax:    +49(0)30-72 62 06 55

Re: parse_cmdline_partitions equivalent for map_info

[Jörn Engel]

On Tue, 22 October 2002 12:39:36 +0200, Frank Neuber wrote:
> > Hmm, somewhere in my collection of yet-to-be-committed patches, I have
> > a variant of the physmap driver that does exactly this and is capable
> > of mapping up to eight independent devices. Interested?
> > 
> Yes, it would be nice to test your code :-)
> Maybe we can do this more generic to add this to cmdline.c.
> What do you think?

Actually, I am not very sure about cmdline.c at all. Is there a single
mapping driver that could not be replaced by physmap.c or mphysmap.c?

The ones I casually looked at did nothing more than hardcoding
addresses and similar stuff. Basically, they condense several config
options into a single one, making the kernel configuration easier.

A quick grep on 2.4.19-ac4 showed that 32 out of 32 mapping drivers
use ioremap.


Ok, enough rambling. Try this patch:
http://wh.fh-wedel.de/~joern/software/kernel/mtd.patch.mphysmap.2.4.19-untested

Should work fine, but I am living without test hardware, feedback or
motivation for six weeks now. "der Entwickler keine Haftung"

Jörn

-- 
Good warriors cause others to come to them and do not go to others.
-- Sun Tzu

Re: parse_cmdline_partitions equivalent for map_info

[Robert Kaiser]

Hi Jörn,

no offense, but ...

Am Dienstag, 22. Oktober 2002 19:29 schrieb Jörn Engel:
> Actually, I am not very sure about cmdline.c at all. Is there a single
> mapping driver that could not be replaced by physmap.c or mphysmap.c?

Not sure about mphysmap.c, but last time I checked, physmap.c was unable to 
deal with configurations where:

  - a set_vpp method is required (see cstm_mips_ixx.c, dilnetpc.c,
    sa1100-flash.c)
  - different kinds of flash chips exist (see sc520cdp.c)
  - bank switched flash is used (see octagon-5066.c, elan-104nc.c,
    sbc_gxx.c,vmax301.c)
  - offset is not a direct index into the flash (see pci.c)


> The ones I casually looked at did nothing more than hardcoding
> addresses and similar stuff. Basically, they condense several config
> options into a single one, making the kernel configuration easier.
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
... which isn't so bad after all ...

BTW: That so many mapping drivers do their own (hardcoded) partitioning 
instead of using cmdline.c is possibly because many of them existed before 
cmdline.c was added and nobody has since bothered to adapt them. And then 
there is also the copy-paste effect (new mapping mapping drivers being 
derived from existing ones, thus copying their shortcomings).


>
> A quick grep on 2.4.19-ac4 showed that 32 out of 32 mapping drivers
> use ioremap.

This doesn't imply that they could all be replaced by physmap.c (see above). 

I agree that many of the existing mapping drivers are basically specialized 
versions of physmap.c. Probably one could make many (most ?) of them obsolete 
by adding just a little bit of functionality to physmap.c, and that would be 
a Good Thing(tm).
But my experience tells me that one should never underestimate the hardware 
designer's creativity when it comes to inventing new weird ways of connecting 
a flash chip to a bus. So, the need for specialized mapping drivers to 
support weird configurations will never vanish completely. 

Given that, I would strongly suggest to encourage the use of cmdline.c 
wherever it makes sense rather than re-inventing that particular wheel and 
implementing it in {m}physmap.c where it would only be available to a subset 
of the possible configurations. Mapping devices and partitioning them are 
independent things and thus they should stay decoupled as they are.


Rob

----------------------------------------------------------------
Robert Kaiser                         email: rkaiser@sysgo.de
SYSGO AG
Am Pfaffenstein 14                    phone: (49) 6136 9948-762
D-55270 Klein-Winternheim / Germany   fax:   (49) 6136 9948-10

Re: parse_cmdline_partitions equivalent for map_info

[Jörn Engel]

On Wed, 23 October 2002 15:25:39 +0200, Robert Kaiser wrote:
> no offense, but ...

When you're right, you're...

> Am Dienstag, 22. Oktober 2002 19:29 schrieb Jörn Engel:
> > Actually, I am not very sure about cmdline.c at all. Is there a single
> > mapping driver that could not be replaced by physmap.c or mphysmap.c?
> 
> Not sure about mphysmap.c, but last time I checked, physmap.c was unable to 
> deal with configurations where:
> 
>   - a set_vpp method is required (see cstm_mips_ixx.c, dilnetpc.c,
>     sa1100-flash.c)
>   - different kinds of flash chips exist (see sc520cdp.c)
>   - bank switched flash is used (see octagon-5066.c, elan-104nc.c,
>     sbc_gxx.c,vmax301.c)
>   - offset is not a direct index into the flash (see pci.c)

Ok, you convinced me. 

> > The ones I casually looked at did nothing more than hardcoding
> > addresses and similar stuff. Basically, they condense several config
> > options into a single one, making the kernel configuration easier.
>                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ... which isn't so bad after all ...

Absolutely not!

> Given that, I would strongly suggest to encourage the use of cmdline.c 
> wherever it makes sense rather than re-inventing that particular wheel and 
> implementing it in {m}physmap.c where it would only be available to a subset 
> of the possible configurations. Mapping devices and partitioning them are 
> independent things and thus they should stay decoupled as they are.

Ok, I should have looked at the code. What mphysmap does is creating
several devices, not partitions. Partitioning should be left out of
mapping, correct.

Thank you for the correction!

Jörn

Re: parse_cmdline_partitions equivalent for map_info

[Frank Neuber]

On Tue, Oct 22, 2002 at 07:08:03PM +0200, Jörn Engel wrote:
Hi Jörn,
thanks for your patch ....
> 
> Ok, enough rambling. Try this patch:
> http://wh.fh-wedel.de/~joern/software/kernel/mtd.patch.mphysmap.2.4.19-untested
But this is not what I need :-(
I would like to pass the physical specification through the kernel
command line.

I think the best place to do this is to add such function to cmdline.c

David, since yesterday I got no mail from the mtd-mailinglist. Maybe it
is my mistake?

Regards,
 Frank

-- 
Dipl.-Ing. Elektrotechnik           convergence / HW
Frank Neuber                        Rosenthalerstr.51 / 10178 Berlin
Email:  neuber@convergence.de           Phone:  +49(0)30-72 62 06 76
WWW:    www.convergence.de              Fax:    +49(0)30-72 62 06 55

Re: parse_cmdline_partitions equivalent for map_info

[Jörn Engel]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

On Wed, 23 October 2002 11:49:28 +0200, Frank Neuber wrote:
> > Ok, enough rambling. Try this patch:
> > http://wh.fh-wedel.de/~joern/software/kernel/mtd.patch.mphysmap.2.4.19-untested
> But this is not what I need :-(

Argl! Sorry, my bad. Try the attached file instead.

Jörn

-- 
The competent programmer is fully aware of the strictly limited size of
his own skull; therefore he approaches the programming task in full
humility, and among other things he avoids clever tricks like the plague. 
-- Edsger W. Dijkstra

[-- Attachment #2: mphysmap.c --]
[-- Type: text/x-csrc, Size: 6028 bytes --]

/*
 * $Id: mphysmap.c,v 1.8 2002/05/27 12:53:53 engeljoe Exp $
 *
 * Normal mappings of multiple chips in physical memory
 */

#include <linux/module.h>
#include <linux/types.h>
#include <linux/kernel.h>
#include <asm/io.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/map.h>
#include <linux/config.h>
#ifdef CONFIG_PROC_FS
#include <linux/proc_fs.h>
#endif

#define error(fmt, args...) printk(KERN_NOTICE "mphysmap: " fmt "\n", ## args);

#define MPHYSMAP_DEVICES 8

#if MPHYSMAP_DEVICES < CONFIG_MTD_MPHYSMAP_AMOUNT
#  error Config.in defines more devices than mphysmap.c can hold
#endif

/*
 * map_priv_1 after ioremap: virtual address to device start
 *           before ioremap: partition parser, see mtd_mphysmap_setup
 * map_priv_2 holds physical address to device start
 */
struct map_info my_map[MPHYSMAP_DEVICES] = {
	{
		name:		CONFIG_MTD_MPHYSMAP1_NAME,
		map_priv_1:	1,
		map_priv_2:	CONFIG_MTD_MPHYSMAP1_START,
		size:		CONFIG_MTD_MPHYSMAP1_LEN,
		buswidth:	CONFIG_MTD_MPHYSMAP1_BUSWIDTH,
#if CONFIG_MTD_MPHYSMAP_AMOUNT > 1
	},{
		name:		CONFIG_MTD_MPHYSMAP2_NAME,
		map_priv_1:	1,
		map_priv_2:	CONFIG_MTD_MPHYSMAP2_START,
		size:		CONFIG_MTD_MPHYSMAP2_LEN,
		buswidth:	CONFIG_MTD_MPHYSMAP2_BUSWIDTH,
#endif
#if CONFIG_MTD_MPHYSMAP_AMOUNT > 2
	},{
		name:		CONFIG_MTD_MPHYSMAP3_NAME,
		map_priv_1:	1,
		map_priv_2:	CONFIG_MTD_MPHYSMAP3_START,
		size:		CONFIG_MTD_MPHYSMAP3_LEN,
		buswidth:	CONFIG_MTD_MPHYSMAP3_BUSWIDTH,
#endif
#if CONFIG_MTD_MPHYSMAP_AMOUNT > 3
	},{
		name:		CONFIG_MTD_MPHYSMAP4_NAME,
		map_priv_1:	1,
		map_priv_2:	CONFIG_MTD_MPHYSMAP4_START,
		size:		CONFIG_MTD_MPHYSMAP4_LEN,
		buswidth:	CONFIG_MTD_MPHYSMAP4_BUSWIDTH,
#endif
	},
};

static int last_device = CONFIG_MTD_MPHYSMAP_AMOUNT - 1;
static int __initdata cmdline_override = 0;

static struct mtd_info *mymtd[MPHYSMAP_DEVICES];

__u8 mphysmap_read8(struct map_info *map, unsigned long ofs)
{
	return __raw_readb(map->map_priv_1 + ofs);
}

__u16 mphysmap_read16(struct map_info *map, unsigned long ofs)
{
	return __raw_readw(map->map_priv_1 + ofs);
}

__u32 mphysmap_read32(struct map_info *map, unsigned long ofs)
{
	return __raw_readl(map->map_priv_1 + ofs);
}

void mphysmap_copy_from(struct map_info *map, void *to, unsigned long from,
		ssize_t len)
{
	memcpy_fromio(to, map->map_priv_1 + from, len);
}

void mphysmap_write8(struct map_info *map, __u8 d, unsigned long adr)
{
	__raw_writeb(d, map->map_priv_1 + adr);
	mb();
}

void mphysmap_write16(struct map_info *map, __u16 d, unsigned long adr)
{
	__raw_writew(d, map->map_priv_1 + adr);
	mb();
}

void mphysmap_write32(struct map_info *map, __u32 d, unsigned long adr)
{
	__raw_writel(d, map->map_priv_1 + adr);
	mb();
}

void mphysmap_copy_to(struct map_info *map, unsigned long to, const void *from, ssize_t len)
{
	memcpy_toio(map->map_priv_1 + to, from, len);
}

/**
 *	tokens are uses as follows:
 *	token[0]: device name
 *	token[1]: start address
 *	token[2]: size
 *	token[3]: buswidth
 *	token[4]: partition parser (0=none, 1=default) (optional, defaults to 1)
 */
static int __init mtd_mphysmap_setup(char *str)
{
	char *token[5], *c;
	int i;

	for (i=0; i<5; i++)
		token[i] = strsep(&str, ",");

	if (str) {
		error("mphysmap: too many arguments passed - device ignored");
		return 1;
	}
	if (!token[3]) {
		error("mphysmap: not enough arguments - device ignored");
		return 1;
	}
	if (!cmdline_override) {
		last_device = -1;
		cmdline_override = 1;
	}

	last_device++;
	my_map[last_device].name = token[0];
	my_map[last_device].map_priv_2 = simple_strtoul(token[1], &c, 0);
	my_map[last_device].size = simple_strtoul(token[2], &c, 0);
	my_map[last_device].buswidth = simple_strtoul(token[3], &c, 0);
	if (token[4]) 
		my_map[last_device].map_priv_1 = simple_strtoul(token[4], &c,0);
	else
		my_map[last_device].map_priv_1 = 1;
	return 1;
}

__setup("mphysmap=", mtd_mphysmap_setup);

#ifdef CONFIG_PROC_FS 

static struct proc_dir_entry *proc_mphysmap;

static int
mphysmap_read_proc(char *page, char **start, off_t off, int count, int *eof,
		void *data_unused)
{
	int i, len;

	len = sprintf(page, "dev: start:     len:\n");
	for (i=0; i<=last_device; i++)
		len += sprintf(page+len, "%2d   0x%08lx 0x%08lx\n",
				MTD_DEVICE(mymtd[i]->minor),
				my_map[i].map_priv_2, my_map[i].size);
	*eof = 1;
	return len;
}

#endif /* CONFIG_PROC_FS */

int __init init_mphysmap(void)
{
	int i, ret = -ENXIO;
	partition_parser *part_pars;

#ifdef CONFIG_PROC_FS
	if ((proc_mphysmap = create_proc_entry("mphysmap", 0, NULL)))
		proc_mphysmap->read_proc = mphysmap_read_proc;
#endif
	for (i=0; i<=last_device; i++) { /*FIXME*/
		if (my_map[i].map_priv_1)
			part_pars = default_parser;
		else
			part_pars = parse_no_partitions;
		printk(KERN_NOTICE "physmap flash device \"%s\": 0x%08lx at "
				"0x%08lx\n", my_map[i].name, my_map[i].size,
				my_map[i].map_priv_2);
		my_map[i].map_priv_1 = (unsigned long)ioremap (
				my_map[i].map_priv_2, my_map[i].size);
		if (!my_map[i].map_priv_1) {
			printk ("Failed to ioremap\n");
			continue;
		}

		my_map[i].read8 = mphysmap_read8;
		my_map[i].read16 = mphysmap_read16;
		my_map[i].read32 = mphysmap_read32;
		my_map[i].copy_from = mphysmap_copy_from;
		my_map[i].write8 = mphysmap_write8;
		my_map[i].write16 = mphysmap_write16;
		my_map[i].write32 = mphysmap_write32;
		my_map[i].copy_to = mphysmap_copy_to;

		mymtd[i] = do_map_probe ("cfi_probe", &my_map[i]);
		if (!mymtd[i]) {
			iounmap ((void*)my_map[i].map_priv_1);
			continue;
		}
		mymtd[i]->module = THIS_MODULE;
		add_mtd_part_device (mymtd[i], part_pars);
		ret = 0; /* We found at least one device */
	}
	return ret;
}

static void __exit cleanup_mphysmap(void)
{
	int i;

#ifdef CONFIG_PROC_FS
	if (proc_mphysmap)
		remove_proc_entry("mphysmap", NULL);
#endif
	for (i=0; i<=last_device; i++) { /*FIXME*/
		if (mymtd[i]) {
			del_mtd_device (mymtd[i]);
			map_destroy (mymtd[i]);
		}
		if (my_map[i].map_priv_1) {
			iounmap ((void*)my_map[i].map_priv_1);
			my_map[i].map_priv_1 = 0;
		}
	}
}

module_init(init_mphysmap);
module_exit(cleanup_mphysmap);

Re: parse_cmdline_partitions equivalent for map_info

[Frank Neuber]

On Wed, Oct 23, 2002 at 12:06:19PM +0200, Jörn Engel wrote:
> On Wed, 23 October 2002 11:49:28 +0200, Frank Neuber wrote:
> > > Ok, enough rambling. Try this patch:
> > > http://wh.fh-wedel.de/~joern/software/kernel/mtd.patch.mphysmap.2.4.19-untested
> > But this is not what I need :-(
> 
> Argl! Sorry, my bad. Try the attached file instead.
Ahh, much more better. This is a good starting point. Thanks!
I try to make this more generic (if time left :-)))

 Frank
-- 
Dipl.-Ing. Elektrotechnik           convergence / HW
Frank Neuber                        Rosenthalerstr.51 / 10178 Berlin
Email:  neuber@convergence.de           Phone:  +49(0)30-72 62 06 76
WWW:    www.convergence.de              Fax:    +49(0)30-72 62 06 55

Re: parse_cmdline_partitions equivalent for map_info

[Jörn Engel]

On Wed, 23 October 2002 12:27:09 +0200, Frank Neuber wrote:
> Ahh, much more better. This is a good starting point. Thanks!

My desk is a mess, my brain is a mess, my patches should be better,
but... sorry for the inconvenience!

> I try to make this more generic (if time left :-)))

Allow me to drop some ideas. Maybe someone else picks those up, before
I do:

- All the mapping drivers in drivers/mtd/maps are just specialized
  and tweaked versions of physmap.c.
- physmap.c was not generic enough. I wrote mphysmap.c for a reason.
- It should be possible to provide a small api that is generic enough
  to rewrite all the mapping drivers in <20 lines of code.

That's the goal. This might be the path:

- physmap.c allows for one range only. Some drivers need multiple
  ranges. Pick mphysmap.c or write something similar.
- Some drivers need to cut out a small range from the middle for the
  bootloader. They then concatenate the range before and after the
  bootloader into one virtual partition. Find a clean solution for
  this.

When physmap.c or any variant is generic enough to do all this, it
needs two or three interfaces:
- Command line, a lot of people should need that.
- A c api for the "got a d-box, hit this options" type files.
(-) Maybe a config language interface. You should cut back on the
  flexibility, though, the Config.in part of the mphysmap patch is
  ugly already, concatenate won't make it any better.

That is my bold vision of how it should be right. Not that I expect it
to be done, but still...

Jörn

-- 
Don't worry about people stealing your ideas. If your ideas are any good,
you'll have to ram them down people's throats.
-- Howard Aiken quoted by Ken Iverson quoted by Jim Horning quoted by
   Raph Levien, 1979

Re: parse_cmdline_partitions equivalent for map_info

[Robert Kaiser]

Am Mittwoch, 23. Oktober 2002 14:07 schrieb Jörn Engel:
> Allow me to drop some ideas. Maybe someone else picks those up, before
> I do:
>
> - All the mapping drivers in drivers/mtd/maps are just specialized
>   and tweaked versions of physmap.c.

Not all, but most of them are (see my previous mail).

> - physmap.c was not generic enough. I wrote mphysmap.c for a reason.
> - It should be possible to provide a small api that is generic enough
>   to rewrite all the mapping drivers in <20 lines of code.

Agreed 98%: I doubt that it will be feasible to cover *all* possible 
configurations in a single generic physmap.c, so the possiblity to drop in a 
specialized mapping driver as a last resort should stay. But generally, I 
agree that there are far too many mapping drivers today.

>
> That's the goal. This might be the path:
>
> - physmap.c allows for one range only. Some drivers need multiple
>   ranges. Pick mphysmap.c or write something similar.

Yep.

> - Some drivers need to cut out a small range from the middle for the
>   bootloader. They then concatenate the range before and after the
>   bootloader into one virtual partition. Find a clean solution for
>   this.

Hmm, I would have thought that mtdconcat.c does this nicely already? (Maybe 
I'm biased since I'm the author of that module, but I'm open to discussion)


>
> When physmap.c or any variant is generic enough to do all this, it
> needs two or three interfaces:
> - Command line, a lot of people should need that.

why not use what is there already (cmdline.c) ?


> - A c api for the "got a d-box, hit this options" type files.

Not sure what you mean by "c api", but generally, MTD is already a nightmare 
to configure for a non-guru, unless there is a specific mapping driver for 
your hardware that you can just click. So this ability to just tickbox your 
platform should definitely stay (or even be extended) when a new concept gets 
introduced.

Rob

----------------------------------------------------------------
Robert Kaiser                         email: rkaiser@sysgo.de
SYSGO AG
Am Pfaffenstein 14                    phone: (49) 6136 9948-762
D-55270 Klein-Winternheim / Germany   fax:   (49) 6136 9948-10

Re: parse_cmdline_partitions equivalent for map_info

[Jörn Engel]

Hello Robert,

we agree on most issues. I have only kept what might be worth a
discussion.

On Wed, 23 October 2002 16:01:35 +0200, Robert Kaiser wrote:
> >   to rewrite all the mapping drivers in <20 lines of code.
> 
> Agreed 98%

> > - Some drivers need to cut out a small range from the middle for the
> >   bootloader. They then concatenate the range before and after the
> >   bootloader into one virtual partition. Find a clean solution for
> >   this.
> 
> Hmm, I would have thought that mtdconcat.c does this nicely already? (Maybe 
> I'm biased since I'm the author of that module, but I'm open to discussion)

I would have to actually take a look, but that should do it. (1)

> > When physmap.c or any variant is generic enough to do all this, it
> > needs two or three interfaces:
> > - Command line, a lot of people should need that.
> 
> why not use what is there already (cmdline.c) ?

Again, see (1)
We have two seperate issues: Creating devices and partitioning those
devices. This is interchangeable in most cases, but not in all.
That was the whole point about my partitioning rewrite.

If I got you right, cmdline.c cares about partitioning a device. But I
worked on hardware that needed to create three devices on flash and
partition two of those. Would that still work easily?

> > - A c api for the "got a d-box, hit this options" type files.
> 
> Not sure what you mean by "c api", but generally, MTD is already a nightmare 
> to configure for a non-guru, unless there is a specific mapping driver for 
> your hardware that you can just click. So this ability to just tickbox your 
> platform should definitely stay (or even be extended) when a new concept gets 
> introduced.

I mean the infrastructure to rewrite 98% of the mapping drivers in <20
lines.

The user should browse through the configuration, see d-box or
whatever, say yes *once* and be happy. That is the best interface next
to autosensing, no doubt.

(1)
About a year ago, I forked David's tree and rewrote the partitioning.
The changes were quite intrusive, but the result looked good to me.
Since then, I wanted to merge the changes back into David's tree. It
never happened, blame goes to both of us.
Sad result is that I continued developing my tree, you all continued
developing David's tree and neither has looked at the other.
Currently, two or three independent groups use my tree, most use
David's tree and noone cares about the situation.
Again, blame everyone. David never looked at my code. I never put it
into nice sizeable pieces for him. And everyone else either dumped
David's tree or ignored mine.

So the result is that I am pretty unaware about current cvs head.
And by now, I don't even care anymore. David is a good developer, but
his problems and my problems don't give a very cooperative mixture.

Jörn

-- 
My second remark is that our intellectual powers are rather geared to
master static relations and that our powers to visualize processes
evolving in time are relatively poorly developed.
-- Edsger W. Dijkstra

Re: parse_cmdline_partitions equivalent for map_info

[Robert Kaiser]

Hi Jörn,

Am Mittwoch, 23. Oktober 2002 16:35 schrieb Jörn Engel:
> >
> > > - Some drivers need to cut out a small range from the middle for the
> > >   bootloader. They then concatenate the range before and after the
> > >   bootloader into one virtual partition. Find a clean solution for
> > >   this.
> >
> > Hmm, I would have thought that mtdconcat.c does this nicely already?
>
> I would have to actually take a look, but that should do it. (1)

The whole reason of the concat layer was that I was facing a situation like 
the one you describe above: BIOS block in the middle of the physical flash 
address range. I had first planned to deal with it in a mapping driver (like 
all the others have done before and after me), but then a discussion with 
David led to my developing the concat layer as a more generic solution.

David initially wanted it to be an extension to the partitioning code because 
he thought that partitioning and concatenating are closely related concepts 
(which seems only logical at first glance), but after looking a little deeper 
into the issue, I preferred to make it a seperate module because:

   1) It turned out that there is really not much a concat layer could
      sensibly share with the partitioning code. They are indeed closely
      related concepts, but only in the sense that each of them is the
      opposite to the other :-)
   2) I wanted it to be decoupled from the partitioning code because I knew
      that your re-write of same was planned to be merged. It's a pity this
      did not happen.

[BTW: The concat layer has a similar problem as cmdline.c: since it was 
introduced relatively late, only few mapping drivers actually use it. And 
since new mapping drivers tend to be created from existing ones in a 
copy-paste manner, this doesn't improve over time...]

>
> > > When physmap.c or any variant is generic enough to do all this, it
> > > needs two or three interfaces:
> > > - Command line, a lot of people should need that.
> >
> > why not use what is there already (cmdline.c) ?
>
> Again, see (1)
> We have two seperate issues: Creating devices and partitioning those
> devices. This is interchangeable in most cases, but not in all.
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Exactly!

> If I got you right, cmdline.c cares about partitioning a device.

cmdline.c just does the tedious work of scanning the kernel commandline 
options and turning the information it finds there into a data structure 
(i.e. the struct mtd_partition that so many mapping drivers have hardcoded). 
This structure can subsequently be passed to the partitioning code. So it is 
basically just a way of specifing a partitioning scheme dynamically through 
the commandline.

It does, however, support multiple MTD devices, i.e. you can specify 
individual partitioning schemes for different MTD devices in a single system.

> But I
> worked on hardware that needed to create three devices on flash and
> partition two of those. Would that still work easily?

The only thing missing here (assuming you want to use physmap.c for this) is 
physmap.c supporting multiple devices. If I got you right, that is what your 
mphysmap.c is about.


>
> > > - A c api for the "got a d-box, hit this options" type files.
> >
> > Not sure what you mean by "c api", but generally, MTD is already a
> > nightmare to configure for a non-guru, unless there is a specific mapping
> > driver for your hardware that you can just click. So this ability to just
> > tickbox your platform should definitely stay (or even be extended) when a
> > new concept gets introduced.
>
> I mean the infrastructure to rewrite 98% of the mapping drivers in <20
> lines.
>
> The user should browse through the configuration, see d-box or
> whatever, say yes *once* and be happy. That is the best interface next
> to autosensing, no doubt.

Fully agreed!

(Hmm, actually I would even *prefer* it to autosensing because I've seen 
autosense code fail so many times ;-)



> (1)
> About a year ago, I forked David's tree and rewrote the partitioning.
> The changes were quite intrusive, but the result looked good to me.
> Since then, I wanted to merge the changes back into David's tree. It
> never happened, blame goes to both of us.
> Sad result is that I continued developing my tree, you all continued
> developing David's tree and neither has looked at the other.
> Currently, two or three independent groups use my tree, most use
> David's tree and noone cares about the situation.
> Again, blame everyone. David never looked at my code. I never put it
> into nice sizeable pieces for him. And everyone else either dumped
> David's tree or ignored mine.

This is really a pity :-(

>
> So the result is that I am pretty unaware about current cvs head.
> And by now, I don't even care anymore. David is a good developer, but
> his problems and my problems don't give a very cooperative mixture.

I wouldn't be so sure about that. It seems to me that many of the problems 
you were facing at the time have been addressed in David's tree in the 
meantime, albeit with different methods.

>From what I gathered up to this point, it seems that a physmap.c that 
supports multiple devices might make a nice addition to David's tree. And, if 
this module could then be generalized to such an extent that it could replace 
-say- 90% of the mapping drivers, this would solve the creeping mapping 
driver inflation that we currently observe.

However, the *big* problem I see with that, is that whoever does this would 
have to analyze every single mapping driver and migrate the relevant bits 
into the new concept. This is tedious, not to speak of the amount of testing 
that needs to be done. I doubt any one person has access to all the vast 
collection of devices for which there exist mapping drivers today.


Rob

----------------------------------------------------------------
Robert Kaiser                         email: rkaiser@sysgo.de
SYSGO AG
Am Pfaffenstein 14                    phone: (49) 6136 9948-762
D-55270 Klein-Winternheim / Germany   fax:   (49) 6136 9948-10

Re: parse_cmdline_partitions equivalent for map_info

[Jörn Engel]

On Wed, 23 October 2002 18:22:49 +0200, Robert Kaiser wrote:
> The whole reason of the concat layer was that I was facing a situation like 
> the one you describe above: BIOS block in the middle of the physical flash 
> address range. I had first planned to deal with it in a mapping driver (like 
> all the others have done before and after me), but then a discussion with 
> David led to my developing the concat layer as a more generic solution.
> 
> David initially wanted it to be an extension to the partitioning code because 
> he thought that partitioning and concatenating are closely related concepts 
> (which seems only logical at first glance), but after looking a little deeper 
> into the issue, I preferred to make it a seperate module because:
> 
>    1) It turned out that there is really not much a concat layer could
>       sensibly share with the partitioning code. They are indeed closely
>       related concepts, but only in the sense that each of them is the
>       opposite to the other :-)

True. All you have to do is grab a couple of partitions and create a
new device from those.
A little performance gets lost, as both concat and partitioning have
to adjust the offset and do some checks. This could be done just once,
but I doubt if the performance gain will be noticeable at all. Keep it
simple, Snoopy.

>    2) I wanted it to be decoupled from the partitioning code because I knew
>       that your re-write of same was planned to be merged. It's a pity this
>       did not happen.

It will. Someday. But when?!?

> > If I got you right, cmdline.c cares about partitioning a device.
> 
> cmdline.c just does the tedious work of scanning the kernel commandline 
> options and turning the information it finds there into a data structure 
> (i.e. the struct mtd_partition that so many mapping drivers have hardcoded). 
> This structure can subsequently be passed to the partitioning code. So it is 
> basically just a way of specifing a partitioning scheme dynamically through 
> the commandline.
> 
> It does, however, support multiple MTD devices, i.e. you can specify 
> individual partitioning schemes for different MTD devices in a single system.

Sounds interesting enough. Worth a close look, when motivation shows
up again.

> > But I
> > worked on hardware that needed to create three devices on flash and
> > partition two of those. Would that still work easily?
> 
> The only thing missing here (assuming you want to use physmap.c for this) is 
> physmap.c supporting multiple devices. If I got you right, that is what your 
> mphysmap.c is about.

Correct.

> > The user should browse through the configuration, see d-box or
> > whatever, say yes *once* and be happy. That is the best interface next
> > to autosensing, no doubt.
> 
> Fully agreed!
> 
> (Hmm, actually I would even *prefer* it to autosensing because I've seen 
> autosense code fail so many times ;-)

Ok, next to working autosensing then. ;-)

> > So the result is that I am pretty unaware about current cvs head.
> > And by now, I don't even care anymore. David is a good developer, but
> > his problems and my problems don't give a very cooperative mixture.
> 
> I wouldn't be so sure about that. It seems to me that many of the problems 
> you were facing at the time have been addressed in David's tree in the 
> meantime, albeit with different methods.

I wouldn't know, how. With current partitioning, imagine two devices,
both with two partitions.
The first device will get minor 0 for the device and minors 1 and 2
for the partitions. The second will get 3, 4 and 5.
Once you create a third partition on the first device, the second will
have 4, 5 and 6. Can you spell trouble?

This was the main issue with my new partitioning code. That and
removing those useless mtd-ro devices, they also cause tons of
problems.

> From what I gathered up to this point, it seems that a physmap.c that 
> supports multiple devices might make a nice addition to David's tree. And, if 
> this module could then be generalized to such an extent that it could replace 
> -say- 90% of the mapping drivers, this would solve the creeping mapping 
> driver inflation that we currently observe.
> 
> However, the *big* problem I see with that, is that whoever does this would 
> have to analyze every single mapping driver and migrate the relevant bits 
> into the new concept. This is tedious, not to speak of the amount of testing 
> that needs to be done. I doubt any one person has access to all the vast 
> collection of devices for which there exist mapping drivers today.

Very true.

Jörn

-- 
Victory in war is not repetitious.
-- Sun Tzu

Re: parse_cmdline_partitions equivalent for map_info

[Robert Kaiser]

Am Sonntag, 27. Oktober 2002 19:00 schrieben Sie:
> On Wed, 23 October 2002 18:22:49 +0200, Robert Kaiser wrote:
> >    1) It turned out that there is really not much a concat layer could
> >       sensibly share with the partitioning code. They are indeed closely
> >       related concepts, but only in the sense that each of them is the
> >       opposite to the other :-)
>
> True. All you have to do is grab a couple of partitions and create a
> new device from those.

Basically yes. As usual, the devil is in the details, like when concatenating 
devices of different type with varying erase block sizes, etc. But this seems 
to work pretty well now (though I'm not sure what will happen if someone 
pushes it, e.g. by concatenating NAND and NOR flash..).

> A little performance gets lost, as both concat and partitioning have
> to adjust the offset and do some checks. This could be done just once,
> but I doubt if the performance gain will be noticeable at all.

Yep. I did make a few benchmarks before I comitted the concat stuff (they 
should be somewhere in the mailing list archive). The difference was not 
measurable.

> Sounds interesting enough. Worth a close look, when motivation shows
> up again.

See. That's the problem. Most of the time, people (including myself, btw :-() 
can't afford to spend the additional time required to bring a new concept 
into shape. All we do most of the time is work on an isolated problem (such 
as "add MTD support for platform XYZ"), but there is hardly enough motivation 
to do a true redesign. So we end up copy-pasting mapping drivers. Sooner or 
later, I fear that this will result in a Big Mess(tm).


> > I wouldn't be so sure about that. It seems to me that many of the
> > problems you were facing at the time have been addressed in David's tree
> > in the meantime, albeit with different methods.
>
> I wouldn't know, how. With current partitioning, imagine two devices,
> both with two partitions.
> The first device will get minor 0 for the device and minors 1 and 2
> for the partitions. The second will get 3, 4 and 5.
> Once you create a third partition on the first device, the second will
> have 4, 5 and 6. Can you spell trouble?

Funny, this concept in the partitioning code to not only create but also 
register devices at the same time bothered me too. So I added a small 
modification to it when I checked in the concat code: If you look at function 
add_mtd_partitions() in mtdpart.c you see that the call to add_mtd_device() 
gets bypassed if the caller has passed an mtd object structure to store the 
device descriptor to. So the caller (usually a mapping driver) can do the 
registration of the device if (and when) it sees fit, and it can thus control 
which minor number (if any) will be assigned to a partition. This small 
change has probably gone largely unnoticed since it is compatible with 
existing mapping drivers. Conceptually, it would have been better to do a 
bold re-write of the partitioning code, but at the time I was already boldy 
writing the concat code, so I was reluctant to open yet another can of worms.



>
> This was the main issue with my new partitioning code. That and
> removing those useless mtd-ro devices, they also cause tons of
> problems.

I'm not sure but I seem to remember that removal of the mtd-ro devices has 
been discussed on this list lately. I haven't been following the discussions 
though.

Rob

----------------------------------------------------------------
Robert Kaiser                         email: rkaiser@sysgo.de
SYSGO AG
Am Pfaffenstein 14                    phone: (49) 6136 9948-762
D-55270 Klein-Winternheim / Germany   fax:   (49) 6136 9948-10

Re: parse_cmdline_partitions equivalent for map_info

[Jörn Engel]

On Mon, 28 October 2002 11:56:55 +0100, Robert Kaiser wrote:
> Basically yes. As usual, the devil is in the details, like when concatenating 
> devices of different type with varying erase block sizes, etc. But this seems 
> to work pretty well now (though I'm not sure what will happen if someone 
> pushes it, e.g. by concatenating NAND and NOR flash..).

This should be possible as well, if someone really wants it. The code
for the ECC-NOR-Chips works just fine with the plain NOR-Chips. And it
used quite a bit of the NAND-code.
Don't use any oob-data for nand, create large erase blocks, do use
write pages of 512 bytes and things should be working.

If someone really wants it. ;-)

> See. That's the problem. Most of the time, people (including myself, btw :-() 
> can't afford to spend the additional time required to bring a new concept 
> into shape. All we do most of the time is work on an isolated problem (such 
> as "add MTD support for platform XYZ"), but there is hardly enough motivation 
> to do a true redesign. So we end up copy-pasting mapping drivers. Sooner or 
> later, I fear that this will result in a Big Mess(tm).

Features cost 20%, cleaning them up costs 80%. That is a lot of work
to be saved in the short term, I know.

> > I wouldn't know, how. With current partitioning, imagine two devices,
> > both with two partitions.
> > The first device will get minor 0 for the device and minors 1 and 2
> > for the partitions. The second will get 3, 4 and 5.
> > Once you create a third partition on the first device, the second will
> > have 4, 5 and 6. Can you spell trouble?
> 
> Funny, this concept in the partitioning code to not only create but also 
> register devices at the same time bothered me too. So I added a small 
> modification to it when I checked in the concat code: If you look at function 
> add_mtd_partitions() in mtdpart.c you see that the call to add_mtd_device() 
> gets bypassed if the caller has passed an mtd object structure to store the 
> device descriptor to. So the caller (usually a mapping driver) can do the 
> registration of the device if (and when) it sees fit, and it can thus control 
> which minor number (if any) will be assigned to a partition. This small 
> change has probably gone largely unnoticed since it is compatible with 
> existing mapping drivers. Conceptually, it would have been better to do a 
> bold re-write of the partitioning code, but at the time I was already boldy 
> writing the concat code, so I was reluctant to open yet another can of worms.

Interesting idea. I still the my approach, which is the same as for
hard disks. n bits describe the device, m bits describe the partition
therein, n+m=8 in our case.
It has always bothered me that a user had no way to tell, whether she
was dealing with a device or a partition. That is quite
counter-intuitive. And the result is bugs caused by false assumptions,
education lessons,... I've spend weeks on that sort of stuff. :-(

> > This was the main issue with my new partitioning code. That and
> > removing those useless mtd-ro devices, they also cause tons of
> > problems.
> 
> I'm not sure but I seem to remember that removal of the mtd-ro devices has 
> been discussed on this list lately. I haven't been following the discussions 
> though.

Yes, that was between David and myself. My second try to get the code
merged, stopped due to lack of test hardware. Two weeks later, uml was
working on my machine, but I didn't receive _any_ feedback meanwhile.
That was, when I lost my motivation, I guess.

Jörn

-- 
Fantasy is more important than knowlegde. Knowlegde is limited,
while fantasy embraces the whole world.
-- Albert Einstein