Re: powerpc_flash_init(), wtf!?

Sergei Shtylyov
2007-05-03T13:02:58Z

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