Messages in this thread | | | Date | Wed, 23 Feb 2011 10:40:32 -0800 | From | David Daney <> | Subject | Re: [RFC PATCH 06/10] MIPS: Octeon: Initialize and fixup device tree. |
| |
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?
> 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.
> 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.
> >> + p->name = "reg"; >> + p->length = sizeof(u32); >> + p->value = vp; >> + >> + *vp = cpu_to_be32((u32)phy); > > phy is already an integer. Why the cast? >
An oversight.
>> + >> + 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.
>> +} >> + >> +static int __init set_mac_addr_prop(struct device_node *n, u64 mac) >> +{ >> + u8 *vp; >> + struct property *old_p; >> + struct property *p = kzalloc(sizeof(struct device_node) + 6, GFP_KERNEL); >> + if (!p) >> + return -ENOMEM; >> + /* The value will immediatly follow the node in memory. */ >> + vp = (u8 *)(&p[1]); >> + p->name = "local-mac-address"; >> + p->length = 6; >> + p->value = vp; >> + >> + vp[0] = (mac>> 40)& 0xff; >> + vp[1] = (mac>> 32)& 0xff; >> + vp[2] = (mac>> 24)& 0xff; >> + vp[3] = (mac>> 16)& 0xff; >> + vp[4] = (mac>> 8)& 0xff; >> + vp[5] = mac& 0xff; >> + >> + old_p = of_find_property(n, "local-mac-address", NULL); >> + if (old_p) >> + prom_remove_property(n, old_p); >> + return prom_add_property(n, p); > > Same comments apply to this function. > >> +} >> + >> +static struct device_node * __init octeon_of_get_child(const struct device_node *parent, >> + int reg_val) >> +{ >> + struct device_node *node = NULL; >> + int size; >> + const __be32 *addr; >> + >> + for (;;) { >> + node = of_get_next_child(parent, node); > > Use for_each_child_of_node() here.
OK.
> >> + if (!node) >> + break; >> + addr = of_get_property(node, "reg",&size); >> + if (addr&& (be32_to_cpu(*addr) == reg_val)) > > be32_to_cpup(addr) >
Right.
>> + break; >> + } >> + return node; >> +} >> + >> +int __init octeon_prune_device_tree(void) >> +{ >> + int i, p, max_port; >> + const char *node_path; >> + char name_buffer[20]; >> + struct device_node *aliases; >> + struct device_node *pip; >> + struct device_node *iface; >> + struct device_node *eth; >> + struct device_node *node; >> + >> + aliases = of_find_node_by_path("/aliases"); >> + if (!aliases) { >> + pr_err("Error: No /aliases node in device tree."); >> + return -EINVAL; >> + } >> + >> + if (OCTEON_IS_MODEL(OCTEON_CN52XX) || OCTEON_IS_MODEL(OCTEON_CN63XX)) >> + max_port = 2; >> + else if (OCTEON_IS_MODEL(OCTEON_CN56XX)) >> + max_port = 1; >> + else >> + max_port = 0; >> + >> + for (i = 0; i< 2; i++) { >> + struct device_node *mgmt; >> + snprintf(name_buffer, sizeof(name_buffer), >> + "ethernet-mgmt%d", i); >> + node_path = of_get_property(aliases, name_buffer, NULL); >> + if (node_path) { >> + mgmt = of_find_node_by_path(node_path); > > of_find_node_by_path() needs to be fixed to also accept alias values > so that a string that starts with a '/' is a full path, but no leading > '/' means start with an alias. This code will lose a level of > indentation if you can make that change to the common code. >
I will consider making that change.
>> + if (!mgmt) >> + continue; >> + if (i>= max_port) { >> + pr_notice("Deleting mgmt%d\n", i); >> + node = of_parse_phandle(mgmt, "phy-handle", 0); >> + if (node) { >> + of_detach_node(node); >> + of_node_put(node); >> + } >> + of_node_put(node); >> + >> + of_detach_node(mgmt); >> + of_node_put(mgmt); >> + } >> + of_node_put(mgmt); >> + } >> + } >> + >> + node_path = of_get_property(aliases, "pip", NULL); >> + if (node_path&& (pip = of_find_node_by_path(node_path))) { >> + for (i = 0; i< 4; i++) { >> + cvmx_helper_interface_enumerate(i); >> + iface = octeon_of_get_child(pip, i); >> + if (!iface) >> + continue; >> + for (p = 0; p< 4; p++) { >> + eth = octeon_of_get_child(iface, p); >> + if (!eth) >> + continue; >> + node = of_parse_phandle(eth, "phy-handle", 0); >> + if (p< cvmx_helper_ports_on_interface(i)) { >> + int phy = cvmx_helper_board_get_mii_address(16 * i + p); >> + if (node&& phy< 0) { >> + struct property *p = of_find_property(eth, "phy-handle", NULL); >> + of_detach_node(node); >> + of_node_put(node); >> + prom_remove_property(eth, p); >> + } > > There is a lot of nesting here; could this be refactored?
Perhaps. It really is a three deep nesting though.
> >> + } else { [...]
>> +} >> +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.
Thanks for looking at it. I will generate another version of the patches.
David Daney
| |