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: Fri, 03 Aug 2007 19:47:43 +0400 [thread overview]
Message-ID: <46B34E1F.5060009@ru.mvista.com> (raw)
In-Reply-To: <20070803031349.GD6418@localhost.localdomain>
Hello.
David Gibson wrote:
>>>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.
Ah, I keep forgetting about implied 1:1 parent/child address
correspondence... :-<
But does it really imply how the (low) address bits of the *child* bus
("ebc" in this case) are connected to the chip? I don't think so...
> 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.
You're lucky to live in the single-endian world. Once you're in bi-endian
one, all kinds of strange mappings become possible. I've seen the MIPS
mapping driver which required swapping flash bytes in s/w in BE mode, i.e.
couldn't be served by physmap, yet that's not all... here's the code of its
map read*() methods:
static __u8 xxx_map_read8(struct map_info *map, unsigned long offs)
{
u16 val;
val = readw(map->map_priv_1 + (offs & ~1));
if (offs & 1)
return ((val >> 8) & 0xff);
else
return (val & 0xff);
}
static __u16 bcm947xx_map_read16(struct map_info *map, unsigned long offs)
{
return readw(map->map_priv_1 + offs);
}
static __u32 bcm947xx_map_read32(struct map_info *map, unsigned long offs)
{
return readl(map->map_priv_1 + offs);
}
... while the simple map (used by physmap) has just __raw_read*() for those?
>>>+ - 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.
Well, "device-width" is not the thing that we care about either. ;-)
>>>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
[...]
>>>+ 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...
I meant that there's no necessity in them.
[...]
>> 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.
Well, if we ever need that indeed... :-)
[...]
>>>@@ -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.
I was thinking about adding "direct-mapped" prop... but maybe adding
"ranges" to the parent node (that's "ebc") would indeed ensure that the flash
is mapped 1:1 to the EBC's parent bus also.
>>>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
> EBC chip select 2, offset 0.
Well, so this node is under some kind of local bus node -- that's good.
Didn't know that the spec allows commas after @...
>>>- 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.
Don't we need "ranges" here, at least from the formal PoV -- as the parent
and child address spaces differ? I know the physmap_of parser doesn't care but
still...
>>>+ };
>>> };
>> 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.
Yes, for example "direct-mapped-cfi" and "direct-mapped-jedec" would have
been better...
> This spec still needs more specific
> description of some properties, at least for JEDEC flashes.
Yes, of course...
WBR, Sergei
next prev parent reply other threads:[~2007-08-03 15:45 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
2007-08-03 3:13 ` David Gibson
2007-08-03 15:47 ` Sergei Shtylyov [this message]
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=46B34E1F.5060009@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