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