linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* Re: Document and implement an improved flash device binding
@ 2007-08-29  6:13 David Gibson
  2007-08-29  8:43 ` Domen Puncer
  2007-09-03 10:23 ` Segher Boessenkool
  0 siblings, 2 replies; 8+ messages in thread
From: David Gibson @ 2007-08-29  6:13 UTC (permalink / raw)
  To: Josh Boyer, linuxppc-dev

for powerpc (v3)
Reply-To: 

This patch replaces the binding for flash chips in
booting-without-of.txt with an clarified and improved version.  It
also makes drivers/mtd/maps/physmap_of.c recognize this new binding.
Finally it revises the Ebony device tree source to use the new binding
as an example.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
Third try at this...

Index: working-2.6/Documentation/powerpc/booting-without-of.txt
===================================================================
--- working-2.6.orig/Documentation/powerpc/booting-without-of.txt	2007-08-29 15:56:19.000000000 +1000
+++ working-2.6/Documentation/powerpc/booting-without-of.txt	2007-08-29 16:10:32.000000000 +1000
@@ -50,7 +50,7 @@
       g) Freescale SOC SEC Security Engines
       h) Board Control and Status (BCSR)
       i) Freescale QUICC Engine module (QE)
-      j) Flash chip nodes
+      j) CFI or JEDEC memory-mapped NOR flash
       k) Global Utilities Block
 
   VII - Specifying interrupt information for devices
@@ -1757,45 +1757,61 @@
 		};
 	};
 
-    j) Flash chip nodes
+   j) CFI or JEDEC memory-mapped NOR flash
 
     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 specify what this flash device is compatible with.
-       Currently, this is most likely to be "direct-mapped" (which
-       corresponds to the MTD physmap mapping driver).
-     - reg : Offset and length of the register set (or memory mapping) for
-       the device.
-     - bank-width : Width of the flash data bus in bytes. Required
-       for the NOR flashes (compatible == "direct-mapped" 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 device and the second one is
-       partition size in bytes with LSB used to signify a read only
-       partition (so, the partition size should always be an even number).
-     - partition-names : The list of concatenated zero terminated strings
-       representing the partition names.
-     - probe-type : The type of probe which should be done for the chip
-       (JEDEC vs CFI actually). Valid ONLY for NOR flashes.
+     - compatible : should contain the specific model of flash chip(s)
+       used, if known, followed by either "cfi-flash" or "jedec-flash"
+     - reg : Address range of the flash chip
+     - bank-width : Width (in bytes) of the flash bank.  Equal to the
+       device width times the number of interleaved chips.
+     - device-width : (optional) Width of a single flash chip.  If
+       omitted, assumed to be equal to 'bank-width'.
+     - #address-cells, #size-cells : Must be present if the flash has
+       sub-nodes representing partitions (see below).  In this case
+       both #address-cells and #size-cells must be equal to 1.
+
+    For JEDEC compatible devices, the following additional properties
+    are defined:
+
+     - vendor-id : Contains the flash chip's vendor id (1 byte).
+     - device-id : Contains the flash chip's device id (1 byte).
+
+    In addition to the information on the flash bank itself, the
+    device tree may optionally contain additional information
+    describing partitions of the flash address space.  This can be
+    used on platforms which have strong conventions about which
+    portions of the flash are used for what purposes, but which don't
+    use an on-flash partition table such as RedBoot.
+
+    Each partition is represented as a sub-node of the flash device.
+    Each node's name represents the name of the corresponding
+    partition of the flash device.
+
+    Flash partitions
+     - reg : The partition's offset and size within the flash bank.
+     - read-only : (optional) This parameter, if present, is a hint to
+       Linux that this flash partition should only be mounted
+       read-only.  This is usually used for flash partitions
+       containing early-boot firmware images or data which should not
+       be clobbered.
 
-   Example:
+    Example:
 
- 	flash@ff000000 {
- 		device_type = "rom";
- 		compatible = "direct-mapped";
- 		probe-type = "CFI";
- 		reg = <ff000000 01000000>;
- 		bank-width = <4>;
- 		partitions = <00000000 00f80000
- 			      00f80000 00080001>;
- 		partition-names = "fs\0firmware";
- 	};
+	flash@ff000000 {
+		compatible = "amd,am29lv128ml", "cfi-flash";
+		reg = <ff000000 01000000>;
+		bank-width = <4>;
+		fs@0 {
+			reg = <0 f80000>;
+		};
+		firmware@f80000 {
+			reg = <f80000 80000>;
+			read-only;
+		};
+	};
 
    k) Global Utilities Block
 
Index: working-2.6/drivers/mtd/maps/physmap_of.c
===================================================================
--- working-2.6.orig/drivers/mtd/maps/physmap_of.c	2007-08-29 15:56:18.000000000 +1000
+++ working-2.6/drivers/mtd/maps/physmap_of.c	2007-08-29 15:56:19.000000000 +1000
@@ -4,6 +4,9 @@
  * Copyright (C) 2006 MontaVista Software Inc.
  * Author: Vitaly Wool <vwool@ru.mvista.com>
  *
+ * Revised to handle newer style flash binding by:
+ *   Copyright (C) 2007 David Gibson, IBM Corporation.
+ *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
  * Free Software Foundation;  either version 2 of the  License, or (at your
@@ -30,56 +33,132 @@
 	struct map_info		map;
 	struct resource		*res;
 #ifdef CONFIG_MTD_PARTITIONS
-	int			nr_parts;
 	struct mtd_partition	*parts;
 #endif
 };
 
-static const char *rom_probe_types[] = { "cfi_probe", "jedec_probe", "map_rom", NULL };
-#ifdef CONFIG_MTD_PARTITIONS
-static const char *part_probe_types[] = { "cmdlinepart", "RedBoot", NULL };
-#endif
-
 #ifdef CONFIG_MTD_PARTITIONS
-static int parse_flash_partitions(struct device_node *node,
-		struct mtd_partition **parts)
+static int parse_obsolete_partitions(struct of_device *dev,
+				     struct physmap_flash_info *info,
+				     struct device_node *dp)
 {
-	int i, plen, retval = -ENOMEM;
-	const  u32  *part;
-	const  char *name;
-
-	part = of_get_property(node, "partitions", &plen);
-	if (part == NULL)
-		goto err;
-
-	retval = plen / (2 * sizeof(u32));
-	*parts = kzalloc(retval * sizeof(struct mtd_partition), GFP_KERNEL);
-	if (*parts == NULL) {
+	int i, plen, nr_parts;
+	const struct {
+		u32 offset, len;
+	} *part;
+	const char *names;
+
+	part = of_get_property(dp, "partitions", &plen);
+	if (!part)
+		return -ENOENT;
+
+	dev_warn(&dev->dev, "Device tree uses obsolete partition map binding\n");
+
+	nr_parts = plen / sizeof(part[0]);
+
+	info->parts = kzalloc(nr_parts * sizeof(struct mtd_partition), GFP_KERNEL);
+	if (!info->parts) {
 		printk(KERN_ERR "Can't allocate the flash partition data!\n");
-		goto err;
+		return -ENOMEM;
 	}
 
-	name = of_get_property(node, "partition-names", &plen);
+	names = of_get_property(dp, "partition-names", &plen);
 
-	for (i = 0; i < retval; i++) {
-		(*parts)[i].offset = *part++;
-		(*parts)[i].size   = *part & ~1;
-		if (*part++ & 1) /* bit 0 set signifies read only partition */
-			(*parts)[i].mask_flags = MTD_WRITEABLE;
+	for (i = 0; i < nr_parts; i++) {
+		info->parts[i].offset = part->offset;
+		info->parts[i].size   = part->len & ~1;
+		if (part->len & 1) /* bit 0 set signifies read only partition */
+			info->parts[i].mask_flags = MTD_WRITEABLE;
 
-		if (name != NULL && plen > 0) {
-			int len = strlen(name) + 1;
+		if (names && (plen > 0)) {
+			int len = strlen(names) + 1;
 
-			(*parts)[i].name = (char *)name;
+			info->parts[i].name = (char *)names;
 			plen -= len;
-			name += len;
-		} else
-			(*parts)[i].name = "unnamed";
+			names += len;
+		} else {
+			info->parts[i].name = "unnamed";
+		}
+
+		part++;
 	}
-err:
-	return retval;
+
+	return nr_parts;
 }
-#endif
+
+static int __devinit process_partitions(struct physmap_flash_info *info,
+					struct of_device *dev)
+{
+	static const char *part_probe_types[]
+		= { "cmdlinepart", "RedBoot", NULL };
+	struct device_node *dp = dev->node, *pp;
+	int nr_parts, i, err;
+
+	/* First look for RedBoot table or partitions on the command
+	 * line, these take precedence over device tree information */
+	nr_parts = parse_mtd_partitions(info->mtd, part_probe_types,
+					&info->parts, 0);
+	if (nr_parts > 0) {
+		add_mtd_partitions(info->mtd, info->parts, err);
+		return 0;
+	}
+
+	/* First count the subnodes */
+	nr_parts = 0;
+	for (pp = dp->child; pp; pp = pp->sibling)
+		nr_parts++;
+
+	if (nr_parts) {
+		info->parts = kzalloc(nr_parts * sizeof(struct mtd_partition),
+				      GFP_KERNEL);
+		if (!info->parts) {
+			printk(KERN_ERR "Can't allocate the flash partition data!\n");
+			return -ENOMEM;
+		}
+
+		for (pp = dp->child, i = 0 ; pp; pp = pp->sibling, i++) {
+			const u32 *reg;
+			int len;
+
+			reg = of_get_property(pp, "reg", &len);
+			if (!reg || (len != 2*sizeof(u32))) {
+				dev_err(&dev->dev, "Invalid 'reg' on %s\n",
+					dp->full_name);
+				kfree(info->parts);
+				info->parts = NULL;
+				return -EINVAL;
+			}
+			info->parts[i].offset = reg[0];
+			info->parts[i].size = reg[1];
+
+			info->parts[i].name =
+				(char *)of_get_property(pp, "name", &len);
+
+			if (of_get_property(pp, "read-only", &len))
+				info->parts[i].mask_flags = MTD_WRITEABLE;
+		}
+	} else {
+		nr_parts = parse_obsolete_partitions(dev, info, dp);
+	}
+
+	if (nr_parts < 0)
+		return nr_parts;
+
+	if (nr_parts > 0)
+		add_mtd_partitions(info->mtd, info->parts, nr_parts);
+	else
+		add_mtd_device(info->mtd);
+
+	return 0;
+}
+#else /* MTD_PARTITIONS */
+static int __devinit process_partitions(struct physmap_flash_info *info,
+					struct device_node *dev)
+{
+	add_mtd_device(info->mtd);
+	return 0;
+}
+#endif /* MTD_PARTITIONS */
 
 static int of_physmap_remove(struct of_device *dev)
 {
@@ -92,7 +171,7 @@
 
 	if (info->mtd != NULL) {
 #ifdef CONFIG_MTD_PARTITIONS
-		if (info->nr_parts) {
+		if (info->parts) {
 			del_mtd_partitions(info->mtd);
 			kfree(info->parts);
 		} else {
@@ -115,16 +194,50 @@
 	return 0;
 }
 
+/* Helper function to handle probing of the obsolete "direct-mapped"
+ * compatible binding, which has an extra "probe-type" property
+ * describing the type of flash probe necessary. */
+static struct mtd_info * __devinit obsolete_probe(struct of_device *dev,
+						  struct map_info *map)
+{
+	struct device_node *dp = dev->node;
+	const char *of_probe;
+	struct mtd_info *mtd;
+	static const char *rom_probe_types[]
+		= { "cfi_probe", "jedec_probe", "map_rom"};
+	int i;
+
+	of_probe = of_get_property(dp, "probe-type", NULL);
+	if (!of_probe) {
+		for (i = 0; i < ARRAY_SIZE(rom_probe_types); i++) {
+			mtd = do_map_probe(rom_probe_types[i], map);
+			if (mtd)
+				return mtd;
+		}
+		return NULL;
+	} else if (strcmp(of_probe, "CFI") == 0) {
+		return do_map_probe("cfi_probe", map);
+	} else if (strcmp(of_probe, "JEDEC") == 0) {
+		return do_map_probe("jedec_probe", map);
+	} else {
+ 		if (strcmp(of_probe, "ROM") != 0)
+			dev_dbg(&dev->dev, "obsolete_probe: don't know probe type "
+				"'%s', mapping as rom\n", of_probe);
+		return do_map_probe("mtd_rom", map);
+	}
+}
+
 static int __devinit of_physmap_probe(struct of_device *dev, const struct of_device_id *match)
 {
 	struct device_node *dp = dev->node;
 	struct resource res;
 	struct physmap_flash_info *info;
-	const char **probe_type;
-	const char *of_probe;
+	const char *probe_type = (const char *)match->data;
 	const u32 *width;
 	int err;
 
+	dev_warn(&dev->dev, "Device tree uses obsolete \"direct-mapped\" "
+		 "flash binding\n");
 
 	if (of_address_to_resource(dp, 0, &res)) {
 		dev_err(&dev->dev, "Can't get the flash mapping!\n");
@@ -174,21 +287,11 @@
 
 	simple_map_init(&info->map);
 
-	of_probe = of_get_property(dp, "probe-type", NULL);
-	if (of_probe == NULL) {
-		probe_type = rom_probe_types;
-		for (; info->mtd == NULL && *probe_type != NULL; probe_type++)
-			info->mtd = do_map_probe(*probe_type, &info->map);
-	} else if (!strcmp(of_probe, "CFI"))
-		info->mtd = do_map_probe("cfi_probe", &info->map);
-	else if (!strcmp(of_probe, "JEDEC"))
-		info->mtd = do_map_probe("jedec_probe", &info->map);
-	else {
- 		if (strcmp(of_probe, "ROM"))
-			dev_dbg(&dev->dev, "map_probe: don't know probe type "
-			"'%s', mapping as rom\n", of_probe);
-		info->mtd = do_map_probe("mtd_rom", &info->map);
-	}
+	if (probe_type)
+		info->mtd = do_map_probe(probe_type, &info->map);
+	else
+		info->mtd = obsolete_probe(dev, &info->map);
+
 	if (info->mtd == NULL) {
 		dev_err(&dev->dev, "map_probe failed\n");
 		err = -ENXIO;
@@ -196,19 +299,7 @@
 	}
 	info->mtd->owner = THIS_MODULE;
 
-#ifdef CONFIG_MTD_PARTITIONS
-	err = parse_mtd_partitions(info->mtd, part_probe_types, &info->parts, 0);
-	if (err > 0) {
-		add_mtd_partitions(info->mtd, info->parts, err);
-	} else if ((err = parse_flash_partitions(dp, &info->parts)) > 0) {
-		dev_info(&dev->dev, "Using OF partition information\n");
-		add_mtd_partitions(info->mtd, info->parts, err);
-		info->nr_parts = err;
-	} else
-#endif
-
-	add_mtd_device(info->mtd);
-	return 0;
+	return process_partitions(info, dev);
 
 err_out:
 	of_physmap_remove(dev);
@@ -221,6 +312,14 @@
 
 static struct of_device_id of_physmap_match[] = {
 	{
+		.compatible	= "cfi-flash",
+		.data		= (void *)"cfi_probe",
+	},
+	{
+		.compatible	= "jedec-flash",
+		.data		= (void *)"jedec_probe",
+	},
+	{
 		.type		= "rom",
 		.compatible	= "direct-mapped"
 	},
Index: working-2.6/arch/powerpc/boot/dts/ebony.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/ebony.dts	2007-08-29 15:56:18.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-08-29 15:56:19.000000000 +1000
@@ -142,13 +142,15 @@
 				interrupt-parent = <&UIC1>;
 
 				small-flash@0,80000 {
-					device_type = "rom";
-					compatible = "direct-mapped";
-					probe-type = "JEDEC";
+					compatible = "jedec-flash";
 					bank-width = <1>;
-					partitions = <0 80000>;
-					partition-names = "OpenBIOS";
 					reg = <0 80000 80000>;
+					#address-cells = <1>;
+					#size-cells = <1>;
+					OpenBIOS@0 {
+						reg = <0 80000>;
+						read-only;
+					};
 				};
 
 				ds1743@1,0 {
@@ -158,14 +160,17 @@
 				};
 
 				large-flash@2,0 {
-					device_type = "rom";
-					compatible = "direct-mapped";
-					probe-type = "JEDEC";
+					compatible = "jedec-flash";
 					bank-width = <1>;
-					partitions = <0 380000
-						      380000 80000>;
-					partition-names = "fs", "firmware";
 					reg = <2 0 400000>;
+					#address-cells = <1>;
+					#size-cells = <1>;
+					fs@0 {
+						reg = <0 380000>;
+					};
+					firmware@380000 {
+						reg = <380000 80000>;
+					};
 				};
 
 				ir@3,0 {


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Document and implement an improved flash device binding
  2007-08-29  6:13 Document and implement an improved flash device binding David Gibson
@ 2007-08-29  8:43 ` Domen Puncer
  2007-08-30  1:18   ` David Gibson
  2007-09-03 10:23 ` Segher Boessenkool
  1 sibling, 1 reply; 8+ messages in thread
From: Domen Puncer @ 2007-08-29  8:43 UTC (permalink / raw)
  To: Josh Boyer, linuxppc-dev

On 29/08/07 16:13 +1000, David Gibson wrote:

<snip>
>  static int __devinit of_physmap_probe(struct of_device *dev, const struct of_device_id *match)
>  {
>  	struct device_node *dp = dev->node;
>  	struct resource res;
>  	struct physmap_flash_info *info;
> -	const char **probe_type;
> -	const char *of_probe;
> +	const char *probe_type = (const char *)match->data;
>  	const u32 *width;
>  	int err;
>  
> +	dev_warn(&dev->dev, "Device tree uses obsolete \"direct-mapped\" "
> +		 "flash binding\n");

This is printed for new binding too :-)

Other than this, it works fine for me.


	Domen

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Document and implement an improved flash device binding
  2007-08-29  8:43 ` Domen Puncer
@ 2007-08-30  1:18   ` David Gibson
  0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2007-08-30  1:18 UTC (permalink / raw)
  To: Domen Puncer; +Cc: linuxppc-dev

On Wed, Aug 29, 2007 at 10:43:41AM +0200, Domen Puncer wrote:
> On 29/08/07 16:13 +1000, David Gibson wrote:
> 
> <snip>
> >  static int __devinit of_physmap_probe(struct of_device *dev, const struct of_device_id *match)
> >  {
> >  	struct device_node *dp = dev->node;
> >  	struct resource res;
> >  	struct physmap_flash_info *info;
> > -	const char **probe_type;
> > -	const char *of_probe;
> > +	const char *probe_type = (const char *)match->data;
> >  	const u32 *width;
> >  	int err;
> >  
> > +	dev_warn(&dev->dev, "Device tree uses obsolete \"direct-mapped\" "
> > +		 "flash binding\n");
> 
> This is printed for new binding too :-)

Duh, put that warning in completely the wrong place.  I was awake,
obviously :-/

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Document and implement an improved flash device binding
  2007-08-29  6:13 Document and implement an improved flash device binding David Gibson
  2007-08-29  8:43 ` Domen Puncer
@ 2007-09-03 10:23 ` Segher Boessenkool
  2007-09-05  2:59   ` David Gibson
  1 sibling, 1 reply; 8+ messages in thread
From: Segher Boessenkool @ 2007-09-03 10:23 UTC (permalink / raw)
  To: David Gibson; +Cc: linuxppc-dev

> +   j) CFI or JEDEC memory-mapped NOR flash
>
>      Flash chips (Memory Technology Devices) are often used for solid 
> state
>      file systems on embedded devices.

Well, almost everything has a NOR flash on it, not just
embedded boards ;-)

> +     - bank-width : Width (in bytes) of the flash bank.  Equal to the
> +       device width times the number of interleaved chips.
> +     - device-width : (optional) Width of a single flash chip.  If
> +       omitted, assumed to be equal to 'bank-width'.

Let's have bank-width optional instead, it's more natural
that way for the common case of just one chip.  Or, you can
say that either is optional.

> +	flash@ff000000 {
> +		compatible = "amd,am29lv128ml", "cfi-flash";
> +		reg = <ff000000 01000000>;
> +		bank-width = <4>;

This is an 8/16-bit part, you need a device-width ;-)

Need #address-cells here for the child nodes:

> +		fs@0 {
> +			reg = <0 f80000>;
> +		};

[big snip]

> +					OpenBIOS@0 {

This show immediately why node name = partition name won't
work out.  You're not supposed to start a node name with a
capital like this.


Segher

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Document and implement an improved flash device binding
  2007-09-03 10:23 ` Segher Boessenkool
@ 2007-09-05  2:59   ` David Gibson
  2007-09-06 13:28     ` Segher Boessenkool
  0 siblings, 1 reply; 8+ messages in thread
From: David Gibson @ 2007-09-05  2:59 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: linuxppc-dev

On Mon, Sep 03, 2007 at 12:23:34PM +0200, Segher Boessenkool wrote:
> >+   j) CFI or JEDEC memory-mapped NOR flash
> >
> >     Flash chips (Memory Technology Devices) are often used for solid 
> >state
> >     file systems on embedded devices.
> 
> Well, almost everything has a NOR flash on it, not just
> embedded boards ;-)

Well, true.

> >+     - bank-width : Width (in bytes) of the flash bank.  Equal to the
> >+       device width times the number of interleaved chips.
> >+     - device-width : (optional) Width of a single flash chip.  If
> >+       omitted, assumed to be equal to 'bank-width'.
> 
> Let's have bank-width optional instead, it's more natural
> that way for the common case of just one chip.  Or, you can
> say that either is optional.

No, I'm disinclined to do that since bank-width is the primary bit of
information that the driver needs.

> >+	flash@ff000000 {
> >+		compatible = "amd,am29lv128ml", "cfi-flash";
> >+		reg = <ff000000 01000000>;
> >+		bank-width = <4>;
> 
> This is an 8/16-bit part, you need a device-width ;-)

Oops - that comes from grabbing a random flash name, combining it with
other example fragments without checking the details.  Added a
device-width.

> Need #address-cells here for the child nodes:

Oops.  Added #a and #s.

> >+		fs@0 {
> >+			reg = <0 f80000>;
> >+		};
> 
> [big snip]
> 
> >+					OpenBIOS@0 {
> 
> This show immediately why node name = partition name won't
> work out.  You're not supposed to start a node name with a
> capital like this.

According to which?

Nonetheless, I've added a label property, and used it for the ebony
tree.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Document and implement an improved flash device binding
  2007-09-05  2:59   ` David Gibson
@ 2007-09-06 13:28     ` Segher Boessenkool
  2007-09-07  1:04       ` David Gibson
  0 siblings, 1 reply; 8+ messages in thread
From: Segher Boessenkool @ 2007-09-06 13:28 UTC (permalink / raw)
  To: David Gibson; +Cc: linuxppc-dev

>>> +     - bank-width : Width (in bytes) of the flash bank.  Equal to 
>>> the
>>> +       device width times the number of interleaved chips.
>>> +     - device-width : (optional) Width of a single flash chip.  If
>>> +       omitted, assumed to be equal to 'bank-width'.
>>
>> Let's have bank-width optional instead, it's more natural
>> that way for the common case of just one chip.  Or, you can
>> say that either is optional.
>
> No, I'm disinclined to do that since bank-width is the primary bit of
> information that the driver needs.

Bzzzzt.  That's not what the device tree is about; it should
describe the hardware, it shouldn't be just a config file for
the current Linux drivers.

Besides, like I said, for the common case where your flash
chips aren't interleaved, it makes way more sense to talk
about device-width than it does to call it bank-width.

>>> +					OpenBIOS@0 {
>>
>> This show immediately why node name = partition name won't
>> work out.  You're not supposed to start a node name with a
>> capital like this.
>
> According to which?

It's just convention, really.

OTOH, spaces and commas and colons and a whole bunch of special
chars are completely disallowed here, so you need...

> Nonetheless, I've added a label property,

...something like that :-)


Segher

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Document and implement an improved flash device binding
  2007-09-06 13:28     ` Segher Boessenkool
@ 2007-09-07  1:04       ` David Gibson
  2007-09-07 13:58         ` Segher Boessenkool
  0 siblings, 1 reply; 8+ messages in thread
From: David Gibson @ 2007-09-07  1:04 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: linuxppc-dev

On Thu, Sep 06, 2007 at 03:28:35PM +0200, Segher Boessenkool wrote:
> >>> +     - bank-width : Width (in bytes) of the flash bank.  Equal to 
> >>> the
> >>> +       device width times the number of interleaved chips.
> >>> +     - device-width : (optional) Width of a single flash chip.  If
> >>> +       omitted, assumed to be equal to 'bank-width'.
> >>
> >> Let's have bank-width optional instead, it's more natural
> >> that way for the common case of just one chip.  Or, you can
> >> say that either is optional.
> >
> > No, I'm disinclined to do that since bank-width is the primary bit of
> > information that the driver needs.
> 
> Bzzzzt.  That's not what the device tree is about; it should
> describe the hardware, it shouldn't be just a config file for
> the current Linux drivers.

Yes, yes, so you've said many times.

But where there are multiple ways of encoding exactly the same
information, I don't see that we can't use driver convenience as a
deciding factor.

> Besides, like I said, for the common case where your flash
> chips aren't interleaved, it makes way more sense to talk
> about device-width than it does to call it bank-width.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Document and implement an improved flash device binding
  2007-09-07  1:04       ` David Gibson
@ 2007-09-07 13:58         ` Segher Boessenkool
  0 siblings, 0 replies; 8+ messages in thread
From: Segher Boessenkool @ 2007-09-07 13:58 UTC (permalink / raw)
  To: David Gibson; +Cc: linuxppc-dev

>>>> Let's have bank-width optional instead, it's more natural
>>>> that way for the common case of just one chip.  Or, you can
>>>> say that either is optional.
>>>
>>> No, I'm disinclined to do that since bank-width is the primary bit of
>>> information that the driver needs.
>>
>> Bzzzzt.  That's not what the device tree is about; it should
>> describe the hardware, it shouldn't be just a config file for
>> the current Linux drivers.
>
> Yes, yes, so you've said many times.

Glad you noticed :-)

> But where there are multiple ways of encoding exactly the same
> information, I don't see that we can't use driver convenience as a
> deciding factor.

But a driver that supports interleaving needs _both_ those pieces
of information, and a driver that doesn't needs the device-width
only.

Sure, the current MTD driver will use some heuristics to guess
the device width, but an interface via which it can get it from
the device tree will have to be added anyway.


Segher

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2007-09-07 13:59 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-08-29  6:13 Document and implement an improved flash device binding David Gibson
2007-08-29  8:43 ` Domen Puncer
2007-08-30  1:18   ` David Gibson
2007-09-03 10:23 ` Segher Boessenkool
2007-09-05  2:59   ` David Gibson
2007-09-06 13:28     ` Segher Boessenkool
2007-09-07  1:04       ` David Gibson
2007-09-07 13:58         ` Segher Boessenkool

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).