Re: powerpc_flash_init(), wtf!?

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

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