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