linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: David Gibson <david@gibson.dropbear.id.au>
To: Segher Boessenkool <segher@kernel.crashing.org>, linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS
Date: Wed, 1 Aug 2007 15:47:51 +1000	[thread overview]
Message-ID: <20070801054751.GM31391@localhost.localdomain> (raw)
In-Reply-To: <20070801050422.GI31391@localhost.localdomain>

On Wed, Aug 01, 2007 at 03:04:22PM +1000, David Gibson wrote:
> On Wed, Aug 01, 2007 at 06:57:33AM +0200, Segher Boessenkool wrote:
> > >> +	UIC0: interrupt-controller0 {
> > >> +		compatible = "ibm,uic-440gp","ibm,uic";
> > >
> > > The first compatible entry should always be the precise model, so in
> > > this case "ibm,uic-440epx".
> > 
> > This isn't really _required_, but it is a very good idea in
> > almost all cases (the exception is for very generic or legacy
> > devices).
> 
> Well, yes.  That's a "should" not a "must" in rfc-speak.
> 
> > > If it is (supposed to be) identical to
> > > the UIC in the 440GP, it can also have an "ibm,uic-440gp" entry, but
> > > since I believe all the UICs are supposed to operate the same, I think
> > > that's implicit in the "ibm,uic" entry.
> > 
> > Sure, but there is no harm in having the better qualified 440gp
> > name in there as well -- bytes are cheap :-)
> > 
> > >> +	SDR0: sdr {
> > >
> > > What is the SDR?
> > >
> > >> +		compatible = "ibm,sdr-440ep";
> > >> +		dcr-reg = <00e 002>;
> > >> +	};
> > >> +
> > >> +	CPR0: cpr {
> > >
> > > And the CPR?
> > 
> > Yeah, better names please -- if possible, something that someone
> > without knowledge of this SoC will understand what it is.
> 
> I think the names are probably ok - I'm assuming they're in keeping
> with the convention I've used of using the same names / abbreviations
> as in the CPU user manual.  I'm asking just for my own information,
> although a comment might not be a bad idea.
> 
> > >> +				nor_flash@0,0 {
> > >> +					device_type = "rom";
> > >> +					compatible = "direct-mapped";
> > >> +					probe-type = "CFI";
> > >
> > > This flash binding needs to be replaced, but I guess that's not really
> > > your problem.
> > 
> > Yeah, that's my problem, thanks for the prod :-)
> 
> Also mine.  I've been home sick the last couple of days, but by way of
> a sharper prod, see my draft work below.  It patches both
> booting-without-of.txt with a revised binding, and implements it in
> the physmap_of driver (which needs renaming, but that's another
> story).  It also revises the ebony device tree as an example.
> 
> This is certainly not complete - it defines none of the extra
> properties that JEDEC chips need (although the mtd drivers'
> defaults/probing seem to cope for ebony).  And there are various other
> ommisions.  Still, it's a starting point - something precise for you
> to flame Segher :-p.

Duh, forgot to attach the actual patch.  Here it is:

Index: working-2.6/Documentation/powerpc/booting-without-of.txt
===================================================================
--- working-2.6.orig/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
+++ working-2.6/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
@@ -1757,45 +1757,23 @@ platforms are moved over to use the flat
 		};
 	};
 
-    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:
+     - compatible : should contain the specific model of flash chip(s) used
+       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'.
+
 
-     - 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.
-
-   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 partitions
+     - reg :
+     - read-only : (optional)
 
    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-07-30 17:07:11.000000000 +1000
+++ working-2.6/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:14.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,17 +33,72 @@ struct physmap_flash_info {
 	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
+static int __devinit handle_of_flash_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++;
+
+	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;
+		const char *name;
+		const void *ro;
+		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);
+			err = -EINVAL;
+			goto fail;
+		}
+		info->parts[i].offset = reg[0];
+		info->parts[i].size = reg[1];
+
+		name = of_get_property(pp, "name", &len);
+		info->parts[i].name = name;
+
+		ro = of_get_property(pp, "read-only", &len);
+		if (ro)
+			info->parts[i].mask_flags = MTD_WRITEABLE;
+	}
+
+	add_mtd_partitions(info->mtd, info->parts, nr_parts);
+	return 0;
+
+ fail:
+	kfree(info->parts);
+	info->parts = NULL;
+	return err;
+}
 
-#ifdef CONFIG_MTD_PARTITIONS
 static int parse_flash_partitions(struct device_node *node,
 		struct mtd_partition **parts)
 {
@@ -79,7 +137,14 @@ static int parse_flash_partitions(struct
 err:
 	return retval;
 }
-#endif
+#else /* MTD_PARTITIONS */
+static int __devinit handle_of_flash_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 +157,7 @@ static int of_physmap_remove(struct of_d
 
 	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,13 +180,45 @@ static int of_physmap_remove(struct of_d
 	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;
 
@@ -174,21 +271,11 @@ static int __devinit of_physmap_probe(st
 
 	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,18 +283,7 @@ static int __devinit of_physmap_probe(st
 	}
 	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);
+	handle_of_flash_partitions(info, dev);
 	return 0;
 
 err_out:
@@ -221,6 +297,14 @@ err_out:
 
 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-07-30 17:07:14.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
@@ -142,13 +142,16 @@
 				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";
+//					partitions = <0 80000>;
+//					partition-names = "OpenBIOS";
 					reg = <0 80000 80000>;
+					#address-cells = <1>;
+					#size-cells = <1>;
+					OpenBIOS@0 {
+						reg = <0 80000>;
+					};
 				};
 
 				ds1743@1,0 {
@@ -158,14 +161,20 @@
 				};
 
 				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";
+//					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

  reply	other threads:[~2007-08-01  5:47 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-07-30 15:06 [PATCH 2/6] PowerPC 440EPx: Sequoia DTS Valentine Barshak
2007-08-01  2:08 ` David Gibson
2007-08-01  4:57   ` Segher Boessenkool
2007-08-01  5:04     ` David Gibson
2007-08-01  5:47       ` David Gibson [this message]
2007-08-02 15:23         ` Sergei Shtylyov
2007-08-03  3:13           ` David Gibson
2007-08-03 15:47             ` Sergei Shtylyov
2007-08-06  4:21               ` David Gibson
2007-08-06 18:37                 ` Sergei Shtylyov
2007-08-06 21:03                   ` Segher Boessenkool
2007-08-06 22:15                   ` Benjamin Herrenschmidt
2007-08-06 23:09                     ` Segher Boessenkool
2007-08-07  3:29                     ` David Gibson
2007-08-07  3:28                   ` David Gibson
2007-08-07 15:43                     ` Scott Wood
2007-08-07 17:01                       ` Segher Boessenkool
2007-08-07 16:43                     ` Segher Boessenkool
2007-08-08  0:35                       ` David Gibson
2007-08-19 12:59                     ` Sergei Shtylyov
2007-08-06 20:54                 ` Segher Boessenkool
2007-08-07  4:12                   ` David Gibson
2007-08-07 16:51                     ` Segher Boessenkool
2007-08-08  1:13                       ` David Gibson
2007-08-09 19:53                         ` Segher Boessenkool
2007-08-10  1:07                           ` David Gibson
2007-08-10 20:48                             ` Segher Boessenkool
2007-08-24 19:10                       ` Sergei Shtylyov
2007-08-24 20:43                         ` Segher Boessenkool
2007-08-06 20:35               ` Segher Boessenkool
2007-08-07  4:09                 ` David Gibson
2007-08-07 16:58                   ` Segher Boessenkool
2007-08-08  0:48                     ` David Gibson
2007-08-06 20:20             ` Segher Boessenkool
2007-08-07  3:35               ` David Gibson
2007-08-06 20:12           ` Segher Boessenkool
2007-08-02 20:18         ` Josh Boyer
2007-08-03  0:49           ` David Gibson
2007-08-03 16:29         ` Sergei Shtylyov
2007-08-06  4:31           ` David Gibson
2007-08-06 20:55             ` Segher Boessenkool
2007-08-06 20:41           ` Segher Boessenkool
2007-08-06 19:59         ` Segher Boessenkool
2007-08-07  3:41           ` David Gibson
2007-08-07 16:33             ` Segher Boessenkool
2007-08-08  1:51               ` David Gibson
2007-08-09 20:00                 ` Segher Boessenkool
2007-08-10  1:11                   ` David Gibson
2007-08-02 20:16       ` Josh Boyer
2007-08-01 14:13   ` Valentine Barshak
2007-08-02  1:00     ` David Gibson
2007-08-02 20:15   ` Josh Boyer
2007-08-06 20:15     ` Segher Boessenkool
2007-08-07  4:11       ` David Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070801054751.GM31391@localhost.localdomain \
    --to=david@gibson.dropbear.id.au \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=segher@kernel.crashing.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).