[PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Thu, Aug 02, 2007 at 07:23:00PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> David Gibson wrote:
> 
> >>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.
> 
>     Let me flame you a bit for starters ;-)

Well, ok then.

> > 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"
> 
>     This "compatible" prop (and the node in whole) doesn't say a
> thing about how the flash is mapped into the CPU address space.  I
> strongly disagree that this node provides enough info to select a
> driver. :-/

To be honest, I'm not sure that describing the mapping is really the
job of the compatible property.  That the flash is mapped into the
address space is kind of implicit in the way the reg interacts with
the parents' ranges property.

Can you describe some of the options for *not* direct mapped flash
chips - I can't reasonably come up with a way of describing the
distinction when I've never seen NOR flash other than direct mapped.

> > +     - 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'.
> 
>     Why then not just introduce the "interleave" prop and obsolete the
> "bank-width"?

Because they're equivalent information, and bank-width is what the
code ends up actually caring about.  I don't see any reason to prefer
giving the interleave.

> > -   Example:
> > -
> > - 	flash at 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";
> > - 	};
> 
>     Why delete the example? :-/

Because it's the old style, and I haven't gotten around to writing a
new example for the new style.

> > +    Flash partitions
> > +     - reg :
> > +     - read-only : (optional)
> 
>     All that would look nice but partition is even less of a device than the
> original "rom" node was... well, who cares now? :-)
> 
> > 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
> [...]
> > @@ -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;
> 
>     We hardly need the above 2 variables.

They're all used...

> > +		int len;
> > +
> > +		reg = of_get_property(pp, "reg", &len);
> > +		if (! reg || (len != 2*sizeof(u32))) {
> 
>     Kill the space after !, please.

Done.

> > +			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;
> > +}
> 
>     Oh, I see that the new partition representation have really simplified
> parsing -- this function is hardly shorter than the old one... :-P

They new format isn't supposed to be simpler to parse: it's supposed
to be more flexible if we ever need to add more per-partition
information than just the offset / size / read-only.

> > -#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;
> 
>     Could get rid of that useless label and goto's in this function, while at
> it...

Yes, haven't really touched this function yet - it needs to be cleaned
up and re-integrated as the fallback method to parse device trees with
the old-style partition description.

> >  }
> > -#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)
> >  {
> > @@ -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. */
> 
>     Too early to obsolete it, I'm afraid... :-)

Hrm..  obsolete != deprecate.  The names designed to discourage use of
it for anything new, not to indicate that it's about to go away.

> > +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;
> [...]
> > @@ -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",
> > +	},
> > +	{
> 
>     This would also trigger on non-linearly mapped CFI or JEDEC
> flashes which is not a good idea -- however, as we're using the MTD
> probing code anyway, it will just fail, so it's not luckily is not a
> fatal design flaw.

Well, if there's nothing else to distinguish them.  Which there isn't
yet, but will need to be: see above about incomplete - helpful
suggestions about how to describe the mapping would be more useful
than merely pointing out the lack.

> >  		.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
> [...]
> > @@ -158,14 +161,20 @@
> >  				};
> >  
> >  				large-flash at 2,0 {
> 
>     Hmm... what does @2,0 mean? :-O

EBC chip select 2, offset 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>;
> 
>     Heh...

Yeah, that bit's a bit ugly, I'll grant you.

> > +					fs at 0 {
> > +						reg = <0 380000>;
> > +					};
> > +					firmware at 380000 {
> > +						reg = <380000 80000>;
> 
>    I guess the "firmware" should have a "read-only" prop...

Possibly.

> > +					};
> >  				};
> 
>     So, I don't see what we're really winning with your changes...

"direct-mapped" is simply not a sufficiently specific compatible
property, no two ways about it.  This spec still needs more specific
description of some properties, at least for JEDEC flashes.

-- 
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