powerpc_flash_init(), wtf!?

powerpc_flash_init(), wtf!?

[David Gibson]

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
flash is on a bus probed with of_platform_bus_probe()
powerpc_flash_init() will create a duplicate of_platform device for
it.  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).

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


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

Unless someone who actually knows how this code was intended to be
used can suggest a more polite way of fixing it.

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

Re: powerpc_flash_init(), wtf!?

[Vitaly Bordug]

On Tue, 1 May 2007 15:18:04 +1000
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.
> 
It should be rewritten then - the way it does init is obsoleted by
the of_platform_bus_probe() now and is confusion-prone. Have to admit I missed
this part while reviewing that rom of_device patch.

[snip]

> 
> Unless someone who actually knows how this code was intended to be
> used can suggest a more polite way of fixing it.
> 
I guess, the idea was for this stuff to be updated once one of the dts inside boot/ would have physmap nodes
added. I have rom/physmap[dts] rehaul in my TODO list, but it has (so far at least) little chance to happen during this merge window. Yet, if someone has suggestions and/or some interest for this to be cured, it will gain priority. Otherwise, I'll replace actual erroneous code with kind of rant that it's up to BSP code to take care of of_devices to be registered, using of_platform_bus_probe() or other way.

-- 
Sincerely, Vitaly

Re: powerpc_flash_init(), wtf!?

[David Gibson]

On Thu, May 03, 2007 at 10:35:34AM +0400, Vitaly Bordug wrote:
> On Tue, 1 May 2007 15:18:04 +1000
> 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.
> > 
> It should be rewritten then - the way it does init is obsoleted by
> the of_platform_bus_probe() now and is confusion-prone. Have to
> admit I missed this part while reviewing that rom of_device patch.

Well, I don't see that it needs rewriting as a unit at all.  The
correct place for probing is in the platform code, no extra rom.c
necessary.

> [snip]
> 
> > 
> > Unless someone who actually knows how this code was intended to be
> > used can suggest a more polite way of fixing it.
> > 
> I guess, the idea was for this stuff to be updated once one of the
> dts inside boot/ would have physmap nodes added. I have
> rom/physmap[dts] rehaul in my TODO list, but it has (so far at
> least) little chance to happen during this merge window. Yet, if
> someone has suggestions and/or some interest for this to be cured,
> it will gain priority. Otherwise, I'll replace actual erroneous code
> with kind of rant that it's up to BSP code to take care of
> of_devices to be registered, using of_platform_bus_probe() or other
> way.

I'm having some trouble parsing that paragraph.  At this stage I don't
see any reason to hold off on tearing out arch/powerpc/sysdev/rom.c,
any necessary changes to replace it will go in the platform code or
other places.

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

Re: powerpc_flash_init(), wtf!?

[Sergei Shtylyov]

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

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

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

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

WBR, Sergei

Re: powerpc_flash_init(), wtf!?

[Sergei Shtylyov]

Hello.

David Gibson wrote:

>>>Unless someone who actually knows how this code was intended to be
>>>used can suggest a more polite way of fixing it.

>>I guess, the idea was for this stuff to be updated once one of the
>>dts inside boot/ would have physmap nodes added. I have
>>rom/physmap[dts] rehaul in my TODO list, but it has (so far at
>>least) little chance to happen during this merge window. Yet, if
>>someone has suggestions and/or some interest for this to be cured,
>>it will gain priority. Otherwise, I'll replace actual erroneous code
>>with kind of rant that it's up to BSP code to take care of
>>of_devices to be registered, using of_platform_bus_probe() or other
>>way.

> I'm having some trouble parsing that paragraph.  At this stage I don't
> see any reason to hold off on tearing out arch/powerpc/sysdev/rom.c,
> any necessary changes to replace it will go in the platform code or

    It doesn't seem a flexible enough approach. We could continue using 
platform devices then.

> other places.

    Any ideas where else?

WBR, Sergei

Re: powerpc_flash_init(), wtf!?

[David Gibson]

On Thu, May 03, 2007 at 04:02:12PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> David Gibson wrote:
> 
> >>>Unless someone who actually knows how this code was intended to be
> >>>used can suggest a more polite way of fixing it.
> 
> >>I guess, the idea was for this stuff to be updated once one of the
> >>dts inside boot/ would have physmap nodes added. I have
> >>rom/physmap[dts] rehaul in my TODO list, but it has (so far at
> >>least) little chance to happen during this merge window. Yet, if
> >>someone has suggestions and/or some interest for this to be cured,
> >>it will gain priority. Otherwise, I'll replace actual erroneous code
> >>with kind of rant that it's up to BSP code to take care of
> >>of_devices to be registered, using of_platform_bus_probe() or other
> >>way.
> 
> > I'm having some trouble parsing that paragraph.  At this stage I don't
> > see any reason to hold off on tearing out arch/powerpc/sysdev/rom.c,
> > any necessary changes to replace it will go in the platform code or
> 
>     It doesn't seem a flexible enough approach. We could continue using 
> platform devices then.

Sorry, I don't follow you.

> > other places.
> 
>     Any ideas where else?

Not really.  I don't immediately see a case where doing it from the
platform code wouldn't be right.

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

Re: powerpc_flash_init(), wtf!?

[Benjamin Herrenschmidt]

>     It doesn't seem a flexible enough approach. We could continue using 
> platform devices then.

The problem is trivial enough tho... who ends up creating an
of_platform_device for that rom node ... to be picked up by the driver.

Creating of_platform devices is platform code responsibility... either
by calling of_platform_bus_probe() (or whatever I called it ...) to
generate them from known bus type or by creating them directly, that
doesn't matter, that's still platform code business.

Ben.

Re: powerpc_flash_init(), wtf!?

[David Gibson]

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

Re: powerpc_flash_init(), wtf!?

[Sergei Shtylyov]

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

Re: powerpc_flash_init(), wtf!?

[Sergei Shtylyov]

Hello.

David Gibson wrote:

>>>>>Unless someone who actually knows how this code was intended to be
>>>>>used can suggest a more polite way of fixing it.

>>>>I guess, the idea was for this stuff to be updated once one of the
>>>>dts inside boot/ would have physmap nodes added. I have
>>>>rom/physmap[dts] rehaul in my TODO list, but it has (so far at
>>>>least) little chance to happen during this merge window. Yet, if
>>>>someone has suggestions and/or some interest for this to be cured,
>>>>it will gain priority. Otherwise, I'll replace actual erroneous code
>>>>with kind of rant that it's up to BSP code to take care of
>>>>of_devices to be registered, using of_platform_bus_probe() or other
>>>>way.

>>>I'm having some trouble parsing that paragraph.  At this stage I don't
>>>see any reason to hold off on tearing out arch/powerpc/sysdev/rom.c,
>>>any necessary changes to replace it will go in the platform code or

>>    It doesn't seem a flexible enough approach. We could continue using 
>>platform devices then.

> Sorry, I don't follow you.

    Well, IIUC, the idea behind the device is to free the platform code of as 
much burden of registering the platform devices itself as possible, isn't it?

>>>other places.

>>    Any ideas where else?

> Not really.  I don't immediately see a case where doing it from the
> platform code wouldn't be right.

    BTW, is it legal/appropriate to specify device (not bus) types for 
of_platform_bus_probe()?

WBR, Sergei

Re: powerpc_flash_init(), wtf!?

[Segher Boessenkool]

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

The "memory" node doesn't describe the RAM devices;
it describes the RAM address space, instead.  You can
have separate nodes for the actual devices.

Now for ROM/flash/NVRAM, nodes _can_ appear directly
under the root, but only if that is where they belong
on your platform (i.e., they sit directly on the "system
bus" (whatever that means on your platform); on most
platforms though, such devices are connected via some
I/O busses, so the nodes should appear under their
respective controllers.

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

Some have it on the LPC bus as an LPC device, some
have it on the LPC bus but accessed with a separate
protocol, some have it attached to another LPC device
(some "superio" typically), some have it attached
directly to the "south bridge".

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

Most "north bridges" have some bits that enable
translation of accesses in the "low bios" area to
the 4GB-minus-a-bit area.  There are many variations
and it all is a big mess :-)


Now, back to the case at hand -- it would be nice to
have a platform-independent way to probe the simple
case -- a single direct-mapped device -- but it isn't
obvious how to make that not clash with the not-so-simple
cases.  A helper function that does the work but is
only called by the platforms that want it would do, I
suppose?


Segher

Re: powerpc_flash_init(), wtf!?

[Segher Boessenkool]

>     BTW, is it legal/appropriate to specify device (not bus) types for
> of_platform_bus_probe()?

In almost all cases you should probe on "name"/"compatible",
and not use "device_type" at all.


Segher

Re: powerpc_flash_init(), wtf!?

[Sergei Shtylyov]

Hello.

Segher Boessenkool wrote:

>>     BTW, is it legal/appropriate to specify device (not bus) types for
>> of_platform_bus_probe()?

> In almost all cases you should probe on "name"/"compatible",

    Probing on name is not a good idea, since those are mostly generic.

> and not use "device_type" at all.

    Heh, SPARC device trees I saw seem to not have this "useless" prop at all. :-)

> Segher

WBR, Sergei

Re: powerpc_flash_init(), wtf!?

[Sergei Shtylyov]

Hello.

Segher Boessenkool wrote:

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

> The "memory" node doesn't describe the RAM devices;
> it describes the RAM address space, instead.  You can
> have separate nodes for the actual devices.

    If you can remember our prior discussion, the "rom" nodes don't describe 
"the actual devices" as well, only their mapping into the address space. ;-)

> Now for ROM/flash/NVRAM, nodes _can_ appear directly
> under the root, but only if that is where they belong
> on your platform (i.e., they sit directly on the "system
> bus" (whatever that means on your platform); on most
> platforms though, such devices are connected via some
> I/O busses, so the nodes should appear under their
> respective controllers.

    Yeah, you're right here, and I've probably misunderstood what "memory" 
node was. In fact, the flash in my system resides on the same local bus as 
RAM, so the proper place would be behind the "lbc" (or whatever -- it doesn't 
exist as yet) node on the "soc" bus.  Do you think I need to go and document 
it as well for such cause? :-]

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

> Some have it on the LPC bus as an LPC device, some
> have it on the LPC bus but accessed with a separate
> protocol,  some have it attached to another LPC device
> (some "superio" typically), some have it attached
> directly to the "south bridge".

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

> Most "north bridges" have some bits that enable
> translation of accesses in the "low bios" area to
> the 4GB-minus-a-bit area.  There are many variations
> and it all is a big mess :-)

    Human perversion knows no limits. O:-)

> Now, back to the case at hand -- it would be nice to
> have a platform-independent way to probe the simple
> case -- a single direct-mapped device -- but it isn't
> obvious how to make that not clash with the not-so-simple
> cases.  A helper function that does the work but is
> only called by the platforms that want it would do, I
> suppose?

    It probably doesn't even worth a helper (since out of those 15 lines, 6 
were pretty useless anyway)

> Segher

WBR, Sergei

Re: powerpc_flash_init(), wtf!?

[Segher Boessenkool]

>>>     BTW, is it legal/appropriate to specify device (not bus) types 
>>> for
>>> of_platform_bus_probe()?
>
>> In almost all cases you should probe on "name"/"compatible",
>
>    Probing on name is not a good idea, since those are mostly generic.

The OF standard tells you to probe on "name" before
probing on "compatible".  If the "name" in a certain
node is generic, that doesn't harm at all, since you
probe for the more specific names only.

>> and not use "device_type" at all.
>
>    Heh, SPARC device trees I saw seem to not have this "useless" prop 
> at all. :-)

Well they have it in the nodes for which they _should_
have it, but not anywhere else no.


Segher

Re: powerpc_flash_init(), wtf!?

[Segher Boessenkool]

>>>     NOR flashes are at the same level as the "memory" node (where 
>>> else you
>>> expect them to appear I wonder?).
>
>> The "memory" node doesn't describe the RAM devices;
>> it describes the RAM address space, instead.  You can
>> have separate nodes for the actual devices.
>
>    If you can remember our prior discussion, the "rom" nodes don't 
> describe "the actual devices" as well, only their mapping into the 
> address space. ;-)

I don't remember that no.  And having a node for the
"ROM address space" isn't useful in the same way as
having one for the "RAM address space" is -- flash
memory is not a resource you randomly hand out to
anyone who wants a piece.  You also need to know some
_specifics_ about a certain ROM device before you can
map it into CPU address space properly.

>> Now for ROM/flash/NVRAM, nodes _can_ appear directly
>> under the root, but only if that is where they belong
>> on your platform (i.e., they sit directly on the "system
>> bus" (whatever that means on your platform); on most
>> platforms though, such devices are connected via some
>> I/O busses, so the nodes should appear under their
>> respective controllers.
>
>    Yeah, you're right here, and I've probably misunderstood what 
> "memory" node was. In fact, the flash in my system resides on the same 
> local bus as RAM, so the proper place would be behind the "lbc" (or 
> whatever -- it doesn't exist as yet) node on the "soc" bus.  Do you 
> think I need to go and document it as well for such cause? :-]

If the "lbc" isn't software visible, you can/should put
the RAM/ROM nodes directly under the SoC node.

This is all just standard considerations, so I don't think
you need to document it separately no.  An example device
tree will help other implementors using your SoC create a
proper device tree, of course.

>> Most "north bridges" have some bits that enable
>> translation of accesses in the "low bios" area to
>> the 4GB-minus-a-bit area.  There are many variations
>> and it all is a big mess :-)
>
>    Human perversion knows no limits. O:-)

Well it's note like there aren't any groovy things on
some PowerPC systems, but x86 definitely wins :-)

>> Now, back to the case at hand -- it would be nice to
>> have a platform-independent way to probe the simple
>> case -- a single direct-mapped device -- but it isn't
>> obvious how to make that not clash with the not-so-simple
>> cases.  A helper function that does the work but is
>> only called by the platforms that want it would do, I
>> suppose?
>
>    It probably doesn't even worth a helper (since out of those 15 
> lines, 6 were pretty useless anyway)

Sure -- but since it is such a common device to have (a
simple NOR boot flash), it would be nice to avoid any
code duplication.  Compare to the serial port and RTC
situation.


Segher

Re: powerpc_flash_init(), wtf!?

[Sergei Shtylyov]

Hello, I wrote:

>>Now for ROM/flash/NVRAM, nodes _can_ appear directly
>>under the root, but only if that is where they belong
>>on your platform (i.e., they sit directly on the "system
>>bus" (whatever that means on your platform); on most
>>platforms though, such devices are connected via some
>>I/O busses, so the nodes should appear under their
>>respective controllers.

>     Yeah, you're right here, and I've probably misunderstood what "memory" 
> node was. In fact, the flash in my system resides on the same local bus as 
> RAM, so the proper place would be behind the "lbc" (or whatever -- it doesn't 
> exist as yet) node on the "soc" bus.  Do you think I need to go and document 
> it as well for such cause? :-]

    No, that probably won't do. MPC85xx SoC bus has ranges = <e0000000 
00100000> and the NOR flash is mapped at 0xff000000, so it seems that it can't 
be located under the "soc" bus (unless that latter has "ranges" prop extended?).

>>Segher

WBR, Sergei

Re: powerpc_flash_init(), wtf!?

[Segher Boessenkool]

>>     Yeah, you're right here, and I've probably misunderstood what 
>> "memory" node was. In fact, the flash in my system resides on the 
>> same local bus as RAM, so the proper place would be behind the "lbc" 
>> (or whatever -- it doesn't exist as yet) node on the "soc" bus.  Do 
>> you think I need to go and document it as well for such cause? :-]
>
>    No, that probably won't do. MPC85xx SoC bus has ranges = <e0000000 
> 00100000> and the NOR flash is mapped at 0xff000000, so it seems that 
> it can't be located under the "soc" bus (unless that latter has 
> "ranges" prop extended?).

If the RAM and/or ROM sit on the SoC bus, the "ranges"
property in the SoC node should be able to translate
their addresses, yes.  You could opt for having the
memory controller a separate device node, as a sibling
of the "soc" node, if that agrees better with your
SoC architecture.  "It all depends".


Segher

Re: powerpc_flash_init(), wtf!?

[Sergei Shtylyov]

Hello.

Segher Boessenkool wrote:

>>>>     NOR flashes are at the same level as the "memory" node (where 
>>>> else you
>>>> expect them to appear I wonder?).

>>> The "memory" node doesn't describe the RAM devices;
>>> it describes the RAM address space, instead.  You can
>>> have separate nodes for the actual devices.

>>    If you can remember our prior discussion, the "rom" nodes don't 
>> describe "the actual devices" as well, only their mapping into the 
>> address space. ;-)

> I don't remember that no.  And having a node for the

    That's a pity. :-)

> "ROM address space" isn't useful in the same way as
> having one for the "RAM address space" is -- flash
> memory is not a resource you randomly hand out to
> anyone who wants a piece.  You also need to know some
> _specifics_ about a certain ROM device before you can
> map it into CPU address space properly.

    Almost all of that is handled by MTD subsys transparently by probing.
What one *must* supply are the bank width and the address mapping (may 
optionally supply a probe type).

>>> Now for ROM/flash/NVRAM, nodes _can_ appear directly
>>> under the root, but only if that is where they belong
>>> on your platform (i.e., they sit directly on the "system
>>> bus" (whatever that means on your platform); on most
>>> platforms though, such devices are connected via some
>>> I/O busses, so the nodes should appear under their
>>> respective controllers.

>>    Yeah, you're right here, and I've probably misunderstood what 
>> "memory" node was. In fact, the flash in my system resides on the same 
>> local bus as RAM, so the proper place would be behind the "lbc" (or 
>> whatever -- it doesn't exist as yet) node on the "soc" bus.  Do you 
>> think I need to go and document it as well for such cause? :-]

> If the "lbc" isn't software visible, you can/should put
> the RAM/ROM nodes directly under the SoC node.

    It has a register set of its own.

>>> Now, back to the case at hand -- it would be nice to
>>> have a platform-independent way to probe the simple
>>> case -- a single direct-mapped device -- but it isn't
>>> obvious how to make that not clash with the not-so-simple
>>> cases.  A helper function that does the work but is
>>> only called by the platforms that want it would do, I
>>> suppose?

>>    It probably doesn't even worth a helper (since out of those 15 
>> lines, 6 were pretty useless anyway)

> Sure -- but since it is such a common device to have (a
> simple NOR boot flash), it would be nice to avoid any
> code duplication.  Compare to the serial port and RTC
> situation.

    UARTs should be registered as of_device by the same bus probing mechanism 
(and there was an attempt at OF based driver, IIRC). 
arch/powerpc/kernel/legacy_serial.c only facilitates the old, platform device 
based approach.

> Segher

WBR, Sergei

Re: powerpc_flash_init(), wtf!?

[Benjamin Herrenschmidt]

On Thu, 2007-05-03 at 18:21 +0200, Segher Boessenkool wrote:
> >     BTW, is it legal/appropriate to specify device (not bus) types for
> > of_platform_bus_probe()?
> 
> In almost all cases you should probe on "name"/"compatible",
> and not use "device_type" at all.

of_platform_bus_probe() is for busses anyway.

Ben.

Re: powerpc_flash_init(), wtf!?

[Benjamin Herrenschmidt]

>     UARTs should be registered as of_device by the same bus probing mechanism 
> (and there was an attempt at OF based driver, IIRC). 

There is more than an "attempt". There is one and it works :-)

> arch/powerpc/kernel/legacy_serial.c only facilitates the old, platform device 
> based approach.

This is a compromise for legacy port (ok, it did grow a bit beyond
legacy stuff but still...) mostly to allow for very early initialisation
of serial ports.

Ultimately, if we generalize of_platform_devices for serial ports,
however, we will not need to create platform devices there anymore.

Ben.

Re: powerpc_flash_init(), wtf!?

[David Gibson]

On Thu, May 03, 2007 at 06:21:35PM +0200, Segher Boessenkool wrote:
> >     BTW, is it legal/appropriate to specify device (not bus) types for
> > of_platform_bus_probe()?
> 
> In almost all cases you should probe on "name"/"compatible",
> and not use "device_type" at all.

I don't think that's what he's getting at.  "compatible" is what we
should use for the device <-> driver matching, but
of_platform_bus_probe() is earlier than that, creating the devices in
the first place.

And no, I don't think it is appropriate to specify device types rather
than bus types there.  Probing devices independent of the bus type
they sit on will always run the risk of conflicting with the bus
type's specific probe logic.

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

Re: powerpc_flash_init(), wtf!?

[David Gibson]

On Thu, May 03, 2007 at 08:07:27PM +0200, Segher Boessenkool wrote:
> >>     Yeah, you're right here, and I've probably misunderstood what 
> >> "memory" node was. In fact, the flash in my system resides on the 
> >> same local bus as RAM, so the proper place would be behind the "lbc" 
> >> (or whatever -- it doesn't exist as yet) node on the "soc" bus.  Do 
> >> you think I need to go and document it as well for such cause? :-]
> >
> >    No, that probably won't do. MPC85xx SoC bus has ranges = <e0000000 
> > 00100000> and the NOR flash is mapped at 0xff000000, so it seems that 
> > it can't be located under the "soc" bus (unless that latter has 
> > "ranges" prop extended?).
> 
> If the RAM and/or ROM sit on the SoC bus, the "ranges"
> property in the SoC node should be able to translate
> their addresses, yes.  You could opt for having the
> memory controller a separate device node, as a sibling
> of the "soc" node, if that agrees better with your
> SoC architecture.  "It all depends".

But if the flash really is on an external bus controlled by a bus
controller on the SoC, it sounds like it should go under that bus
bridge.  In which case the SoC would need another range in its ranges
property.

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

Re: powerpc_flash_init(), wtf!?

[Vitaly Bordug]

On Thu, 03 May 2007 22:29:13 +1000
Benjamin Herrenschmidt wrote:

> 
> >     It doesn't seem a flexible enough approach. We could continue
> > using platform devices then.
> 
> The problem is trivial enough tho... who ends up creating an
> of_platform_device for that rom node ... to be picked up by the
> driver.
> 
> Creating of_platform devices is platform code responsibility... either
> by calling of_platform_bus_probe() (or whatever I called it ...) to
> generate them from known bus type or by creating them directly, that
> doesn't matter, that's still platform code business.
> 
Still, there is code duplication problem.

So, assuming we'd need 6 lines of code to do it properly in platform code,
we may face  tens of different targets that require exactly the same process to 
provide mtd of_devices stuff.

The hard part of it is apparently how the respective flash space should be remapped
(b/c it is depending on HW design), but since we really care only about things that may be
encoded into device tree, there is really an option to utilize devtree power and effectiveness,
I think.


-- 
Sincerely, Vitaly

Re: powerpc_flash_init(), wtf!?

[David Gibson]

On Fri, May 04, 2007 at 04:30:56AM +0400, Vitaly Bordug wrote:
> On Thu, 03 May 2007 22:29:13 +1000
> Benjamin Herrenschmidt wrote:
> 
> > 
> > >     It doesn't seem a flexible enough approach. We could continue
> > > using platform devices then.
> > 
> > The problem is trivial enough tho... who ends up creating an
> > of_platform_device for that rom node ... to be picked up by the
> > driver.
> > 
> > Creating of_platform devices is platform code responsibility... either
> > by calling of_platform_bus_probe() (or whatever I called it ...) to
> > generate them from known bus type or by creating them directly, that
> > doesn't matter, that's still platform code business.
> > 
> Still, there is code duplication problem.
> 
> So, assuming we'd need 6 lines of code to do it properly in platform
> code, we may face tens of different targets that require exactly the
> same process to provide mtd of_devices stuff.

So make a helper function.

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

Re: powerpc_flash_init(), wtf!?

[Segher Boessenkool]

>> If the RAM and/or ROM sit on the SoC bus, the "ranges"
>> property in the SoC node should be able to translate
>> their addresses, yes.  You could opt for having the
>> memory controller a separate device node, as a sibling
>> of the "soc" node, if that agrees better with your
>> SoC architecture.  "It all depends".
>
> But if the flash really is on an external bus controlled by a bus
> controller on the SoC, it sounds like it should go under that bus
> bridge.  In which case the SoC would need another range in its ranges
> property.

Yeah, it all depends on the specific SoC device.
I don't know which one we're talking about here
so I can't give any more specific advice.


Segher

Re: powerpc_flash_init(), wtf!?

[Sergei Shtylyov]

Hello.

David Gibson wrote:

>>>>    Yeah, you're right here, and I've probably misunderstood what 
>>>>"memory" node was. In fact, the flash in my system resides on the 
>>>>same local bus as RAM, so the proper place would be behind the "lbc" 
>>>>(or whatever -- it doesn't exist as yet) node on the "soc" bus.  Do 
>>>>you think I need to go and document it as well for such cause? :-]

>>>   No, that probably won't do. MPC85xx SoC bus has ranges = <e0000000 
>>>00100000> and the NOR flash is mapped at 0xff000000, so it seems that 
>>>it can't be located under the "soc" bus (unless that latter has 
>>>"ranges" prop extended?).

>>If the RAM and/or ROM sit on the SoC bus, the "ranges"
>>property in the SoC node should be able to translate
>>their addresses, yes.  You could opt for having the
>>memory controller a separate device node, as a sibling
>>of the "soc" node, if that agrees better with your
>>SoC architecture.  "It all depends".

> But if the flash really is on an external bus controlled by a bus
> controller on the SoC, it sounds like it should go under that bus
> bridge.  In which case the SoC would need another range in its ranges
> property.

    Erm, how multiple memory ranges are supposed to work? Aren't the addresses 
in the "reg" property of subnodes relative to the "ranges" property?

WBR, Sergei

Re: powerpc_flash_init(), wtf!?

[Segher Boessenkool]

>> But if the flash really is on an external bus controlled by a bus
>> controller on the SoC, it sounds like it should go under that bus
>> bridge.  In which case the SoC would need another range in its ranges
>> property.
>
>    Erm, how multiple memory ranges are supposed to work? Aren't the 
> addresses in the "reg" property of subnodes relative to the "ranges" 
> property?

If the address space of a child bus is a subset (perhaps
after some mapping) of the address space of the parent bus,
the "ranges" property in the parent node describes that
subset (and mapping).  If the address spaces are identical,
this is expressed by an empty "ranges" property.  If there
is no such mapping (indirect register access, for example),
this is expressed by the absence of a "ranges" property.

There can be multiple ranges in a "ranges" property, that's
why it's called "ranges" and not "range".  A common example
is the "ranges" property in PCI bus nodes (if those have
both a memory and legacy I/O range; for host bridges, this
means that both those ranges are direct mapped).

I hope this answered your question, it was a bit vague.


Segher

Re: powerpc_flash_init(), wtf!?

[Mark A. Greer]

Sorry, I've lost the thread in my inbox so I can't reply to it directly.

Is something like this what everyone agreed upon?

Mark
--

diff --git a/arch/powerpc/platforms/embedded6xx/prpmc2800.c b/arch/powerpc/platforms/embedded6xx/prpmc2800.c
index d9db135..44c3144 100644
--- a/arch/powerpc/platforms/embedded6xx/prpmc2800.c
+++ b/arch/powerpc/platforms/embedded6xx/prpmc2800.c
@@ -20,6 +20,7 @@
 #include <asm/system.h>
 #include <asm/time.h>
 #include <asm/kexec.h>
+#include <asm/of_platform.h>
 
 #include <mm/mmu_decl.h>
 
@@ -134,6 +135,18 @@ void prpmc2800_show_cpuinfo(struct seq_file *m)
 }
 
 /*
+ * Register a platform device for MTD.
+ */
+static int __init prpmc2800_register_mtd(void)
+{
+	struct device_node *np;
+
+	np = of_find_compatible_node(NULL, "rom", "direct-mapped");
+	of_platform_device_create(np, np->name, NULL);
+}
+arch_initcall(prpmc2800_register_mtd);
+
+/*
  * Called very early, device-tree isn't unflattened
  */
 static int __init prpmc2800_probe(void)

Re: powerpc_flash_init(), wtf!?

[David Gibson]

On Wed, May 23, 2007 at 02:57:35PM -0700, Mark A. Greer wrote:
> Sorry, I've lost the thread in my inbox so I can't reply to it directly.
> 
> Is something like this what everyone agreed upon?

That's the right approach if your flash device isn't on a bus that's
already being scanned with of_platform_bus_probe().

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