[PATCH] adding ROM chips to device tree: respin

Hello folks,

inlined below is the patch which adds support for flash device descriptions to the OF device tree. It's inspired by and partially borrowed from Sergei's patch which can be found at http://patchwork.ozlabs.org/linuxppc/patch?id=6526 but arranges things in a different way.
It should be used together with the corresponding MTD layer extension the current version of which was recently posted to linux-mtd list for discussion and can be found at http://lists.infradead.org/pipermail/linux-mtd/2006-November/016700.html.
In fact, currently this description can handle only flash devices mapped into memory in a linear way.
This patch is a respin of the patch which can be found at http://ozlabs.org/pipermail/linuxppc-embedded/2006-November/025013.html with comments from Sergei and Josh taken into account.

 Documentation/powerpc/booting-without-of.txt |   37 +++++++++++++++++++++++++++
 arch/powerpc/sysdev/Makefile                 |    1 
 arch/powerpc/sysdev/rom.c                    |   31 ++++++++++++++++++++++
 3 files changed, 69 insertions(+)
 
Signed-off-by: Vitaly Wool <redacted>

Index: powerpc/Documentation/powerpc/booting-without-of.txt
===================================================================

--- powerpc.orig/Documentation/powerpc/booting-without-of.txt
+++ powerpc/Documentation/powerpc/booting-without-of.txt

@@ -6,6 +6,8 @@
     IBM Corp.
 (c) 2005 Becky Bruce <becky.bruce at freescale.com>,
     Freescale Semiconductor, FSL SOC and 32-bit additions
+(c) 2006 MontaVista Software, Inc.
+    Flash chip node definition
 
    May 18, 2005: Rev 0.1 - Initial draft, no chapter III yet.
 

@@ -1693,6 +1695,41 @@ platforms are moved over to use the flat
 		};
 	};
 
+    g) Flash chip nodes
+
+    Flash chips (Memory Technology Devices) are often used for solid state
+    file systems on embedded devices.
+
+    Required properties:
+
+     - device_type : has to be "rom"
+     - compatible : Should be the name of the MTD driver. Currently, this is
+       most likely to be "physmap".
+     - memory_space : Offset and length of the register set for the device.
+     - bank-width : Width of the flash data bus in bytes. Required
+       for the NOR flashes (compatible == "physmap" and others) ONLY.
+
+    Recommended properties :
+
+     - partitions : Several pairs of 32-bit values where the first value is
+       partition's offset from the start of the MTD device and the second
+       one is partition size in bytes with LSB used to signify a read only
+       partititon (so, the parition size should always be an even number).
+     - partition-names : The list of concatenated zero terminated strings
+       representing the partition names.
+
+   Example:
+
+ 	flash@ff000000 {
+ 		device_type = "rom";
+ 		compatible = "physmap";
+ 		memory_space = <ff000000 01000000>;
+ 		bank-width = <4>;
+ 		partitions = <00000000 00f80000
+ 			      00f80000 00080001>;
+ 		partition-names = "fs\0firmware";
+ 	};
+
    More devices will be defined as this spec matures.
 
 

Index: powerpc/arch/powerpc/sysdev/Makefile
===================================================================

--- powerpc.orig/arch/powerpc/sysdev/Makefile
+++ powerpc/arch/powerpc/sysdev/Makefile

@@ -12,6 +12,7 @@ obj-$(CONFIG_MMIO_NVRAM)	+= mmio_nvram.o
 obj-$(CONFIG_FSL_SOC)		+= fsl_soc.o
 obj-$(CONFIG_TSI108_BRIDGE)	+= tsi108_pci.o tsi108_dev.o
 obj-$(CONFIG_QUICC_ENGINE)	+= qe_lib/
+obj-$(CONFIG_MTD)		+= rom.o
 
 ifeq ($(CONFIG_PPC_MERGE),y)
 obj-$(CONFIG_PPC_I8259)		+= i8259.o

Index: powerpc/arch/powerpc/sysdev/rom.c
===================================================================

--- /dev/null
+++ powerpc/arch/powerpc/sysdev/rom.c

@@ -0,0 +1,31 @@
+/*
+ * arch/powerpc/sysdev/flash.c
+ *
+ * Flash memory registration
+ *
+ * (C) 2006 MontaVista Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+
+#include <asm/of_device.h>
+
+static int __init powerpc_flash_init(void)
+{
+	struct device_node *node = NULL;
+	int num = 0;
+
+	/*
+	 * We care only about physmap devices now as there's no
+	 * description defined for other ROM types yet
+	 */
+	while ((node = of_find_compatible_node(node, "rom", "physmap"))
+			!= NULL) {
+		of_platform_device_create(node, "physmap-flash", NULL);
+		++num;
+	}
+	return 0;
+}
+
+arch_initcall(powerpc_flash_init);

Hello.

Vitaly Wool wrote:

   NAK. There's no need to define an extra property where "reg" should be used.

+     - bank-width : Width of the flash data bus in bytes. Required
+       for the NOR flashes (compatible == "physmap" and others) ONLY.

   Then it can not be called a *required* property I think.

   Not true. The description only says that it's *most probably* compatible with "physmap", that's all. I don't see why we have to limit ourselves here.

   Why are you counting them, can you say?

WBR, Sergei

quoted

+     - memory_space : Offset and length of the register set for the device.

    NAK. There's no need to define an extra property where "reg" should be used.

The register set is actually not there and depends on flash chip type.
So using regs here is misleading.

quoted

+
+	/*
+	 * We care only about physmap devices now as there's no
+	 * description defined for other ROM types yet
+	 */

    Not true. The description only says that it's *most probably* compatible 
with "physmap", that's all. I don't see why we have to limit ourselves here.

Effectively we care about NOR chips and similar which are
memory-mapped. 

Vitaly

Hello.

Vitaly Wool wrote:

quoted

quoted

+     - memory_space : Offset and length of the register set for the device.

   NAK. There's no need to define an extra property where "reg" should be used.

The register set is actually not there and depends on flash chip type.
So using regs here is misleading.

   This is an I/O resource on the parent bus and using the property other than "reg" will be misleading.  That's the way this spec has it -- "reg" is used even for the PHY chip numbering on MDIO bus...

quoted

quoted

+
+	/*
+	 * We care only about physmap devices now as there's no
+	 * description defined for other ROM types yet
+	 */

quoted

   Not true. The description only says that it's *most probably* compatible with "physmap", that's all. I don't see why we have to limit ourselves here.

Effectively we care about NOR chips and similar which are
memory-mapped.

   So what? How "physmap" follows from this?

Vitaly

WBR, Sergei

В Ср, 08/11/2006 в 00:25 +0300, Sergei Shtylyov пишет:

Hello.

Vitaly Wool wrote:

quoted

quoted

quoted

+     - memory_space : Offset and length of the register set for the device.

   NAK. There's no need to define an extra property where "reg" should be used.

quoted

The register set is actually not there and depends on flash chip type.
So using regs here is misleading.

    This is an I/O resource on the parent bus and using the property other 
than "reg" will be misleading.  That's the way this spec has it -- "reg" is 
used even for the PHY chip numbering on MDIO bus...

So what? Lemme remind you that the actual registers *doesn't start* at
the specified "start" so using regs is really a bad idea IMHO.

quoted

quoted

quoted

+
+	/*
+	 * We care only about physmap devices now as there's no
+	 * description defined for other ROM types yet
+	 */

quoted

quoted

   Not true. The description only says that it's *most probably* compatible 
with "physmap", that's all. I don't see why we have to limit ourselves here.

quoted

Effectively we care about NOR chips and similar which are
memory-mapped. 

    So what? How "physmap" follows from this?

Okay, probably we can go you way naming of_device by what it's
compatible with. So that "physmap" compatible would be called
"physmap-flash", "nand"-compatible would be called "nand-flash" etc.
Does that work for you?

Vitaly

Hello.

Vitaly Wool wrote:

quoted

quoted

quoted

quoted

+     - memory_space : Offset and length of the register set for the device.

  NAK. There's no need to define an extra property where "reg" should be used.

quoted

The register set is actually not there and depends on flash chip type.
So using regs here is misleading.

   This is an I/O resource on the parent bus and using the property other than "reg" will be misleading.  That's the way this spec has it -- "reg" is used even for the PHY chip numbering on MDIO bus...

So what? Lemme remind you that the actual registers *doesn't start* at
the specified "start" so using regs is really a bad idea IMHO.

   We don't have any actual registers (at least "physmap" doesn't know about them anyway) I think, just a memory range. What registers are you talking about?

quoted

quoted

quoted

quoted

+
+	/*
+	 * We care only about physmap devices now as there's no
+	 * description defined for other ROM types yet
+	 */

quoted

quoted

quoted

  Not true. The description only says that it's *most probably* compatible with "physmap", that's all. I don't see why we have to limit ourselves here.

quoted

quoted

Effectively we care about NOR chips and similar which are
memory-mapped.

quoted

   So what? How "physmap" follows from this?

Okay, probably we can go you way naming of_device by what it's
compatible with. So that "physmap" compatible would be called
"physmap-flash", "nand"-compatible would be called "nand-flash" etc.

   Actually, Generic Names spec tells to use the most generic user-parsable names, just like I used initally ("flash")...

Does that work for you?

   No. Getting rid of "physmap" completely and using of_find_node_by_type() does. :-)

Vitaly

WBR, Sergei

    We don't have any actual registers (at least "physmap" doesn't know about 
them anyway) I think, just a memory range. What registers are you talking about?

We do have registers there, in query mode. The CFI registers.

quoted

Okay, probably we can go you way naming of_device by what it's
compatible with. So that "physmap" compatible would be called
"physmap-flash", "nand"-compatible would be called "nand-flash" etc.

    Actually, Generic Names spec tells to use the most generic user-parsable 
names, just like I used initally ("flash")...

quoted

Does that work for you?

    No. Getting rid of "physmap" completely and using of_find_node_by_type() 
does. :-)

That's effectively what I'm talking about. Find the device by type and
register it with the name which depends on what it's compatible with.

Vitaly

Hello.

Vitaly Wool wrote:

quoted

   We don't have any actual registers (at least "physmap" doesn't know about them anyway) I think, just a memory range. What registers are you talking about?

We do have registers there, in query mode. The CFI registers.

   Aren't they accessed thru the range we're giving to the mapping layer? What else do you need? Who cares about the exact offsets at the mapping level (or at the level of the bus)?

quoted

quoted

Okay, probably we can go you way naming of_device by what it's
compatible with. So that "physmap" compatible would be called
"physmap-flash", "nand"-compatible would be called "nand-flash" etc.

quoted

   Actually, Generic Names spec tells to use the most generic user-parsable names, just like I used initally ("flash")...

quoted

quoted

Does that work for you?

quoted

   No. Getting rid of "physmap" completely and using of_find_node_by_type() does. :-)

That's effectively what I'm talking about. Find the device by type and
register it with the name which depends on what it's compatible with.

   I stand enlightened. It have finally occured to me that this of_device stuff wasn't worth the effort of using it and moving the code into drivers/mtd/. This move doesn't buy you much -- you still have to walk the device tree stupidly in search for the "known" devices... unless you're making up the names for of_platform bus like this. And what for? Those names aren't used to select the correct driver anyway... Crap. :-(
   We could also try using "device_type" or "name" properties to name the of_device -- this name doesn't matter much anyway...

Vitaly

WBR, Sergei

Hello, I wrote:

    We could also try using "device_type" or "name" properties to name the of_device -- this name doesn't matter much anyway...

   Now, after looking thru some code, I think using the "name" prop is indeed the way to go...

WBR, Sergei