MTD Maps driver for Sharp LH7a40x

I've removed the ununsed, if-def's and commented, code from the driver.

As for the C_MAPS loop, there doesn't seem to be a good way to remove it. Even though there is only one 'chip', we still the map_info structure for the simple_map_init() and friends. The number of code code bytes to be saved is nomimal. The change would be to remove the array references. How do they make the code 'confusing'?

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/06/13 14:15:25-07:00 [email protected]
# Removed unused code for whole-device mapping.
#

# drivers/mtd/maps/lpd7a400-flash.c
#   2004/06/13 14:15:15-07:00 [email protected] +2 -14
#   Removed unused code for whole-device mapping.
# 
# drivers/mtd/maps/lpd7a400-flash.c
#   2004/06/04 13:25:08-07:00 [email protected] +171 -0
# 
# drivers/mtd/maps/lpd7a400-flash.c
#   2004/06/04 13:25:08-07:00 [email protected] +0 -0
#   BitKeeper file /m/elf/home/z/embedded/linux/drivers/mtd/maps/lpd7a400-flash.c
# 
# drivers/mtd/maps/Makefile
#   2004/06/04 13:25:08-07:00 [email protected] +1 -0
#   Makefile change to include LPD7a40x MTD/Maps driver.
# 
# drivers/mtd/maps/Kconfig
#   2004/06/04 13:25:08-07:00 [email protected] +8 -0
#   New configuration option for MTD/Maps driver for Logic PD LPD7a40x boards.
# 
diff -Nru a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
--- a/drivers/mtd/maps/Kconfig	Sun Jun 13 14:22:06 2004
+++ b/drivers/mtd/maps/Kconfig	Sun Jun 13 14:22:06 2004
@@ -421,6 +421,14 @@
 	  Excalibur XA10 Development Board. If you are building a kernel
 	  for on of these boards then you should say 'Y' otherwise say 'N'.

+config MTD_LPD7A40X
+ tristate "CFI Flash device mapped on Logic PD LH7A40X-10 Card Engines"
+ depends on (MACH_LPD7A400 || MACH_LPD7A404) && MTD_CFI_INTELEXT && MTD_PARTITIONS
+ help

+	  This provides a driver for the on-board flash of the Logic PD
+	  LH7A40X-10 Card Engines.  Static partition mappings
+	  correspond to the BLOB loader's layout.

+
config MTD_FORTUNET
tristate "CFI Flash device mapped on the FortuNet board" depends on ARM && MTD_CFI && MTD_PARTITIONS && SA1100_FORTUNET

diff -Nru a/drivers/mtd/maps/Makefile b/drivers/mtd/maps/Makefile
@@ -56,3 +56,4 @@
 obj-$(CONFIG_MTD_ARCTIC)	+= arctic-mtd.o
 obj-$(CONFIG_MTD_H720X)		+= h720x-flash.o
 obj-$(CONFIG_MTD_IXP4XX)	+= ixp4xx.o
+obj-$(CONFIG_MTD_LPD7A40X)	+= lpd7a400-flash.o

That's not a question of code bytes.

Why does this code need a for (i = 0; i < C_MAPS;...) loop, if there is only one chip which has to be detected, neglected or whatever and C_MAPS is therefor 1 ?

In fact the whole driver could be replaced by command line options.

Thomas

You originally stated that you thought it made the driver 'confusing'. I'm not seeing how this makes it confusing.

So can the memory map. The presence of a command line option doesn't make it a good idea to use it.

Let me put this a different way. Are you saying that you'll apply the patch if I remove the loop?

Cheers.

How about just using physmap mapping driver and keep all your
board specific stuff to your board directory? See include/linux/mtd/physmap.h
for more details.

Jun

Jun,

Thanks for jumping in. That's what I was trying to point out.

In fact most of the board drivers in drivers/mtd/maps could / should go away. The reason to have one is crappy hardware, unusual chip constellations or other anomalities.

The common case can just be solved by using the physmap driver and the command line partition parser. Maybe we should think about an extension to physmap so the board support code can supply an appropriate partitioning scheme along with the chip description.

I was thinking about simliar support for the NAND driver too, but there it turns out that the anomalities are given per design, although it should not be an unsolvable problem.

Thomas
_____________________________________________________________________
From slash dot org
"When customers are visiting, engineers are not allowed to wear ties. 
That way the customer can tell who is the engineer and who is the 
salesman (and therefore whom to believe.). Ties cut off blood flow 
to the brain, making it easier for the salesmen to do their jobs." 
_____________________________________________________________________
linutronix - competence in embedded & realtime linux
http://www.linutronix.de
mail: [email protected]

I've chatted with Russell King about this, too. I think we agree that board-specific code would be a bad idea. The better solution appears to be something on the command line. I'm going to look into it. For now, consider this PATCH discarded.

Yes, for the common case where we dont have special functions neccecary the commandline resp. ATAG (unfortunately only for ARM) is the right place. This works if the only information which is neccecary is

base address, size and buswidth.

Russell, can you please give define two ATAG numbers for this purpose ? ATAG_MTD_PHYSMAP and
ATAG_MTD_PARTITION
I would be happy to provide the data structures and parsers to make this work.

BUT,

For chips which need Vpp setting for programming and erasing this is rather impossible. Same applies to NAND drivers, as the NAND interface can hardly be specified board independent on the commandline.

For general purpose boards (PCI, PC104 whatever) drivers/mtd/ is surely the right place. For embedded boards I'm not sure if it makes sense to have all this code in the mtd subsystem or if it would be better to have this in the board specific code in arch/xxx/boards/myboard.c which is there anyway.

Another possiblity would be to include a hardware related header file in the physmap driver and the to be written counterpart for NAND.

#define BOARD_SPECIFIC_MTD_FUNCTIONS
#include <asm/hardware.h>

asm/hardware.h includes board_hardware.h if available.

Inside board_hardware.h then we would have

#ifdef BOARD_SPECIFIC_MTD_FUNCTIONS
board_mtd_functionA(){}
board_mtd_functionB(){}

(nand)pyhsmap chip = {
	.address = xxxx;
	.....
	.functionA = board_mtd_functionA;
	.functionA = board_mtd_functionB;
}

#endif

Not all arch's provide a hardware.h file but this should be a solvable problem. For ARM this should work right out of the box.

I know it looks weird in the first place, but it could be one of the possible solutions to solve the board support mess. Russell uses a similar approach for the timer init and interrupt handler for the various subarchs of ARM since long.

Any thoughts ?

Thomas
_____________________________________________________________________
From slash dot org
"When customers are visiting, engineers are not allowed to wear ties. 
That way the customer can tell who is the engineer and who is the 
salesman (and therefore whom to believe.). Ties cut off blood flow 
to the brain, making it easier for the salesmen to do their jobs." 
_____________________________________________________________________
linutronix - competence in embedded & realtime linux
http://www.linutronix.de
mail: [email protected]

ATAG is only ARM because Linux doesn't like binary interfaces - and that's one of the reasons I don't want to see it expanding more and more. See the "IDE support for lh7a40x" on the linux-arm-kernel list for a discussion on this topic.

I've already thrown out passing partition information via ATAGs years ago, and the solution which came from that was the mtdparts= command line argument. Now that we have that, there is no reason for ATAG_MTD_PARTITION.

You may want to take a look at how integrator-flash.c handles this aspect. The handling of VPP and write enables is platform specific and requires some code to do it, so obviously we can't encode that into an ATAG or command line argument.

Note how integrator-flash.c is used on the Integrator/AP, Integrator/CP and Versatile/PB platforms, and notice the way the platform support code is separate in arch/arm/mach-integrator/integrator_*.c and arch/arm/mach-versatile/core.c. I think this is a good model for what you're trying to achieve.

Also note that the size of the flash doesn't have to exactly match the size of flash fitted to the device - it just has to be of sufficient size to cover the largest flash which may be fitted to your device. (or, if you have a way to discover the fitted flash size without too much hastle, then you could use that.)

Eww, no thanks. We've recently killed that in the PXA tree. hardware.h is included by 99% of the kernel include files which means any modification to it or included files causes the whole kernel to rebuild - which takes long enough on x86 with gcc 3 already.

The timer init is a mess and Deepak's been cleaning that up, so please don't use the current structure as a justification.

I think you're better off using integrator-flash.c as is, and just supplying the relevent platform device structure and associated code.

Ok, sounds reasonable. I did not follow this thread, as the topic was out of my primary interest.

Yep. I was not aware of this. But it would have been my first choice too to put this into the board support files / directories which are available / neccecary for embedded systems anyway.

I know. For NAND we use the chip ID to do that anyway.

I just said that ist possible, but weird. You're definitly right to make this go away.

So a final conclusion would be:

1. Add commandline parsing for flashbase,size,buswidth which covers the case where no special code is neccecary to access the chips.

2. Use the already existing platform model, which can be retrieved from your work in arch/arm/mach-xxxx

3. Extend the generic NAND driver to provide a simple mechanism to make 2.) work for NAND too

4. Obsolete the board specific mapping drivers in drivers/mtd/maps and drivers/mtd/nand when this is done.

Did I miss(understand) anything ?

Thomas
_____________________________________________________________________
From slash dot org
"When customers are visiting, engineers are not allowed to wear ties. 
That way the customer can tell who is the engineer and who is the 
salesman (and therefore whom to believe.). Ties cut off blood flow 
to the brain, making it easier for the salesmen to do their jobs." 
_____________________________________________________________________
linutronix - competence in embedded & realtime linux
http://www.linutronix.de
mail: [email protected]

I don't have strong views on this, but if the platform model is going to be used (so that we get the VPP and WP enable code), there seems to be little point in separating this out into the boot loader.

Other than that, it seems fine.