Subject: Re: powerpc_flash_init(), wtf!?

From: David Gibson <[email-masked]>
Date: Thu, 3 May 2007 22:30:55 +1000

On Thu, May 03, 2007 at 03:47:36PM +0400, Sergei Shtylyov wrote:
> 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...

> >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().

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.

> > 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. :-<
> 
> >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.

> >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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson