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


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