From: Sergei Shtylyov <sshtylyov@ru•mvista.com>
To: David Gibson <david@gibson•dropbear.id.au>
Cc: linuxppc-dev@ozlabs•org
Subject: Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS
Date: Thu, 02 Aug 2007 19:23:00 +0400 [thread overview]
Message-ID: <46B1F6D4.3070707@ru.mvista.com> (raw)
In-Reply-To: <20070801054751.GM31391@localhost.localdomain>
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 ;-)
> 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. :-/
> + - 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"?
> - 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";
> - };
Why delete the example? :-/
> + 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.
> + int len;
> +
> + reg = of_get_property(pp, "reg", &len);
> + if (! reg || (len != 2*sizeof(u32))) {
Kill the space after !, please.
> + 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
> -#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...
> }
> -#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... :-)
> +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.
> .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@2,0 {
Hmm... what does @2,0 mean? :-O
> - 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...
> + fs@0 {
> + reg = <0 380000>;
> + };
> + firmware@380000 {
> + reg = <380000 80000>;
I guess the "firmware" should have a "read-only" prop...
> + };
> };
So, I don't see what we're really winning with your changes...
WBR, Sergei
next prev parent reply other threads:[~2007-08-02 15:21 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
2007-08-02 15:23 ` Sergei Shtylyov [this message]
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=46B1F6D4.3070707@ru.mvista.com \
--to=sshtylyov@ru$(echo .)mvista.com \
--cc=david@gibson$(echo .)dropbear.id.au \
--cc=linuxppc-dev@ozlabs$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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