From: Sergei Shtylyov <sshtylyov@ru•mvista.com>
To: David Gibson <david@gibson•dropbear.id.au>
Cc: linuxppc-dev@ozlabs•org
Subject: Re: powerpc_flash_init(), wtf!?
Date: Thu, 03 May 2007 17:04:33 +0400 [thread overview]
Message-ID: <4639DDE1.40904@ru.mvista.com> (raw)
In-Reply-To: <20070503123055.GE26659@localhost.localdomain>
Hello.
David Gibson wrote:
>>>powerpc_flash_init(), the only function in arch/powerpc/sysdev/rom.c,
>>>goes through the device tree finding anything with device_type=="rom"
>>>and creating of_platform devices for them, which will be picked up by
>>>the physmap_of mtd driver. This has two serious conceptual errors and
>>>one bad implementation error which is quite an accomplishment for 15
>>>lines of code.
>>>Most seriously, this "find all roms" approach to probing is
>>>fundamentally incompatible with the normal way of probing for
>>>of_platform devices, to wit, using of_platform_bus_probe(). If a
>> We weren't aware of the of_platform.c work when writing the MTD support.
>> Note that this function usually probes only the specified set of (SoC)
>>busses, none of which usully contains NOR flash (which is located at the
>>root level).
> The root level? Um... I don't think so...
"Trust me". :-)
NOR flashes are at the same level as the "memory" node (where else you
expect them to appear I wonder?).
>>>flash is on a bus probed with of_platform_bus_probe()
>>>powerpc_flash_init() will create a duplicate of_platform device for it.
>> Flash on a SoC bus? Well, that's more likely to happen for NAND.
>>But generally, I'd agree with you.
> Well, on Ebony, the (NOR) flash is on the bus controlled by the
> 440gp's external bus controller (EBC). So it's not an SoC bus as
> such, but it's still a "dumb bus" (to use BenH's terminology) which
> can be suitably probed by of_platform_bus_probe().
Interesting...
> I believe the arrangement is similar for most other 4xx systems. More
> PC or desktop like systems sometimes have boot flash connected to the
> south bridge, which I believe puts it on the ISA bus, topologically
> speaking.
Not exactly. Boot flash is mapped beyond ISA address space on 386+ -- at
the top of 4GB (where the "reset vector" is). Although it may be dual mapped
below 1MB as well (I'm starting to forget x86 :-).
>>>powerpc_flash_init() could also mistakenly probe roms which
>>>appear on other random busses which should use their own probe logic
>>>instead of going straight off the device tree (admittedly flash is
>>>unlikely to appear on such a bus).
>> Well, if you consider NAND...
>>>Also, it uses the device node's name without unit address as the
>>>of_platform device's name. So if a bus somewhere has two flash
>>>devices named, say "flash@0" and "flash@800000", the device code will
>>>give a stack dump during boot as powerpc_flash_init() attempts to
>>>register them both under the name "flash".
>> Well, we didn't think about 2 flashes named the same way. :-/
>>>I observe that none of the dts files actually present in the kernel
>>>tree use physmap_of's format for describing flash devices (and
>>>therefore don't use this code). I'm therefore rather tempted to
>> Which means I still haven't submitted the patch. :-<
I hope to post a patch soon.
>>>simply blow arch/powerpc/sysdev/rom.c away, and anyone out-of-tree
>>>relying on this code will have to fix their platform probing code to
>>>create the flash of_platform devices properly.
>> You mean creating the "rom" devices from the platform-specific code?
>>I doubt that it's really a flexible approach...
> Since it's handled on a per-platform basis, it's more-or-less by
> definition more flexible than the current broken approach.
I'm worried about the code duplication.
>>>Unless someone who actually knows how this code was intended to be
>>>used can suggest a more polite way of fixing it.
>> Well, probably it needs to only look up the root bus...
> Really, truly on the root bus? Even so I don't think such a probe
> should be conducted as an initcall whenever CONFIG_MTD is set. A
> helper function invoked from the platform code might be reasonable.
Yeah, I agree. Probably doesn't even worth a function since for most
cases there's only one flash.
WBR, Sergei
next prev parent reply other threads:[~2007-05-03 13:02 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-05-01 5:18 powerpc_flash_init(), wtf!? David Gibson
2007-05-03 6:35 ` Vitaly Bordug
2007-05-03 7:03 ` David Gibson
2007-05-03 12:02 ` Sergei Shtylyov
2007-05-03 12:22 ` David Gibson
2007-05-03 13:28 ` Sergei Shtylyov
2007-05-03 16:21 ` Segher Boessenkool
2007-05-03 16:59 ` Sergei Shtylyov
2007-05-03 17:25 ` Segher Boessenkool
2007-05-03 21:37 ` Benjamin Herrenschmidt
2007-05-03 23:49 ` David Gibson
2007-05-03 12:29 ` Benjamin Herrenschmidt
2007-05-04 0:30 ` Vitaly Bordug
2007-05-04 1:28 ` David Gibson
2007-05-03 11:47 ` Sergei Shtylyov
2007-05-03 12:30 ` David Gibson
2007-05-03 13:04 ` Sergei Shtylyov [this message]
2007-05-03 16:20 ` Segher Boessenkool
2007-05-03 17:17 ` Sergei Shtylyov
2007-05-03 17:35 ` Segher Boessenkool
2007-05-03 18:19 ` Sergei Shtylyov
2007-05-03 21:44 ` Benjamin Herrenschmidt
2007-05-03 17:53 ` Sergei Shtylyov
2007-05-03 18:07 ` Segher Boessenkool
2007-05-03 23:56 ` David Gibson
2007-05-04 12:14 ` Segher Boessenkool
2007-05-05 17:36 ` Sergei Shtylyov
2007-05-05 20:19 ` Segher Boessenkool
-- strict thread matches above, loose matches on Subject: below --
2007-05-23 21:57 Mark A. Greer
2007-05-24 0:56 ` 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=4639DDE1.40904@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