lkml.org 
[lkml]   [2011]   [Feb]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH 06/10] MIPS: Octeon: Initialize and fixup device tree.
    On Wed, Feb 23, 2011 at 10:40:32AM -0800, David Daney wrote:
    > On 02/23/2011 09:41 AM, Grant Likely wrote:
    > >On Tue, Feb 22, 2011 at 12:57:50PM -0800, David Daney wrote:
    > >>Signed-off-by: David Daney<ddaney@caviumnetworks.com>
    > >>---
    > >> arch/mips/Kconfig | 2 +
    > >> arch/mips/cavium-octeon/octeon-platform.c | 280 +++++++++++++++++++++++++++++
    > >> arch/mips/cavium-octeon/setup.c | 17 ++
    > >> 3 files changed, 299 insertions(+), 0 deletions(-)
    > >
    > >I've got an odd feeling of foreboding about this patch. It makes me
    > >nervous, but I can't articulate why yet. Gut-wise I'd rather see the
    > >device tree pruned/fixed up before it gets unflattened,
    >
    > I chose to work on the unflattened form because there were already
    > functions to do it. I didn't see anything that would make
    > manipulating the flattened form easy.
    >
    > I agree that working on the unflattened form would be best. At a
    > minium the /proc/device-tree structure would better reflect reality.
    >
    > What do you think about adding some helper functions to
    > drivers/of/fdt.c for the manipulation of the flattened form?

    It would probably be easier/safer to link libfdt into the kernel
    proper. It's already used in the powerpc bootwrapper, and there has
    been talk about replacing some of fdt.c with libfdt. See
    scripts/dtc/libfdt

    >
    > >or for the
    > >kernel to have a separate .dtb linked in for each legacy platform.
    >
    > I think there are too many variants to make this viable.

    Out of curiosity, how many variants?

    btw, did you know about the dtc '/include/' functionality? It is
    possible to set up .dts include files that represent a SoC and can be
    modified by the .dts files that include them. See
    arch/powerpc/boot/dts/*5200*.dts

    >
    > > I
    > >need to think about this some more....
    > >
    > >I've made some comments below anyway.
    >
    > And I will respond. Although if I end up modifying the flattened
    > form, it will all change.
    >
    > >
    > [...]
    > >>+
    > >>+static int __init set_phy_addr_prop(struct device_node *n, int phy)
    > >>+{
    > >>+ u32 *vp;
    > >>+ struct property *old_p;
    > >>+ struct property *p = kzalloc(sizeof(struct device_node) + sizeof(u32), GFP_KERNEL);
    > >>+ if (!p)
    > >>+ return -ENOMEM;
    > >>+ /* The value will immediatly follow the node in memory. */
    > >>+ vp = (u32 *)(&p[1]);
    > >
    > >This is unsafe (I was on the losing end of an argument when I tried to
    > >do exactly the same thing). If you want to allocate 2 things with one
    > >appended to the other, then you need to define a structure
    > >with the two element in it and allocate the size of that structure.
    >
    > Weird. alloc_netdev() does this, so it is not unheard of.

    Not unheard of, but still bad practise.

    > >>+ old_p = of_find_property(n, "reg", NULL);
    > >>+ if (old_p)
    > >>+ prom_remove_property(n, old_p);
    > >>+ return prom_add_property(n, p);
    > >
    > >Would it not be more efficient to change the value in the existing reg
    > >property instead of doing this allocation song-and-dance?
    > >
    >
    > I think I did it this way to try to get /proc/device-tree to reflect
    > the new value.

    Sounds like a bug in /proc/device-tree. :-) /proc/device-tree should
    be pointing directly at the device tree property itself. I'd be
    surprised if modifying the data of 'reg' didn't show up there.

    > >>+}
    > >>+arch_initcall(octeon_fix_device_tree);
    > >
    > >Calling this from an initcall really makes me nervous. I'm worried
    > >about ordering issues. Why can this code not be part of the prune
    > >routine above?
    > >
    >
    > Again, done to try to make /proc/device-tree reflect reality.

    yeah, /proc/device-tree should not be driving design decisions. Let's
    try to fix it instead.

    g.



    \
     
     \ /
      Last update: 2011-02-23 19:53    [W:2.355 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site