lkml.org 
[lkml]   [2011]   [Feb]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC PATCH 06/10] MIPS: Octeon: Initialize and fixup device tree.
    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, or for the
    kernel to have a separate .dtb linked in for each legacy platform. I
    need to think about this some more....

    I've made some comments below anyway.

    >
    > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
    > index 4baf7f2..a8fc970 100644
    > --- a/arch/mips/Kconfig
    > +++ b/arch/mips/Kconfig
    > @@ -699,6 +699,7 @@ config CAVIUM_OCTEON_SIMULATOR
    > select SYS_SUPPORTS_HIGHMEM
    > select SYS_SUPPORTS_HOTPLUG_CPU
    > select SYS_HAS_CPU_CAVIUM_OCTEON
    > + select OF_DYNAMIC
    > help
    > The Octeon simulator is software performance model of the Cavium
    > Octeon Processor. It supports simulating Octeon processors on x86
    > @@ -715,6 +716,7 @@ config CAVIUM_OCTEON_REFERENCE_BOARD
    > select SYS_SUPPORTS_HOTPLUG_CPU
    > select SYS_HAS_EARLY_PRINTK
    > select SYS_HAS_CPU_CAVIUM_OCTEON
    > + select OF_DYNAMIC
    > select SWAP_IO_SPACE
    > select HW_HAS_PCI
    > select ARCH_SUPPORTS_MSI
    > diff --git a/arch/mips/cavium-octeon/octeon-platform.c b/arch/mips/cavium-octeon/octeon-platform.c
    > index cecaf62..428de0d 100644
    > --- a/arch/mips/cavium-octeon/octeon-platform.c
    > +++ b/arch/mips/cavium-octeon/octeon-platform.c
    > @@ -13,10 +13,14 @@
    > #include <linux/usb.h>
    > #include <linux/dma-mapping.h>
    > #include <linux/module.h>
    > +#include <linux/slab.h>
    > #include <linux/platform_device.h>
    > +#include <linux/of_platform.h>
    >
    > #include <asm/octeon/octeon.h>
    > #include <asm/octeon/cvmx-rnm-defs.h>
    > +#include <asm/octeon/cvmx-helper.h>
    > +#include <asm/octeon/cvmx-helper-board.h>
    >
    > static struct octeon_cf_data octeon_cf_data;
    >
    > @@ -440,6 +444,282 @@ device_initcall(octeon_ohci_device_init);
    >
    > #endif /* CONFIG_USB */
    >
    > +static struct of_device_id __initdata octeon_ids[] = {
    > + { .type = "soc", },
    > + { .compatible = "simple-bus", },
    > + {},
    > +};
    > +
    > +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.

    > + p->name = "reg";
    > + p->length = sizeof(u32);
    > + p->value = vp;
    > +
    > + *vp = cpu_to_be32((u32)phy);

    phy is already an integer. Why the cast?

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

    > +}
    > +
    > +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.

    > + if (!node)
    > + break;
    > + addr = of_get_property(node, "reg", &size);
    > + if (addr && (be32_to_cpu(*addr) == reg_val))

    be32_to_cpup(addr)

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

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

    > + } else {
    > + pr_notice("Deleting Ethernet %x:%x\n", i, p);
    > + if (node) {
    > + of_detach_node(node);
    > + of_node_put(node);
    > + }
    > + of_detach_node(eth);
    > + of_node_put(eth);
    > + }
    > + of_node_put(node);
    > + of_node_put(eth);
    > + }
    > + of_node_put(iface);
    > + }
    > + of_node_put(pip);
    > + }
    > +
    > + /* I2C */
    > + if (OCTEON_IS_MODEL(OCTEON_CN52XX) ||
    > + OCTEON_IS_MODEL(OCTEON_CN63XX) ||
    > + OCTEON_IS_MODEL(OCTEON_CN56XX))
    > + max_port = 2;
    > + else
    > + max_port = 1;
    > +
    > + for (i = 0; i < 2; i++) {
    > + struct device_node *i2c;
    > + snprintf(name_buffer, sizeof(name_buffer),
    > + "i2c%d", i);
    > + node_path = of_get_property(aliases, name_buffer, NULL);
    > + if (node_path) {
    > + i2c = of_find_node_by_path(node_path);
    > + if (!i2c)
    > + continue;
    > + if (i >= max_port) {
    > + pr_notice("Deleting i2c%d\n", i);
    > +
    > + of_detach_node(i2c);
    > + of_node_put(i2c);
    > + }
    > + of_node_put(i2c);
    > + }
    > + }
    > +
    > + of_node_put(aliases);
    > + return 0;
    > +}
    > +
    > +int __init octeon_fix_device_tree(void)
    > +{
    > + int i, p;
    > + int rv;
    > + const char *node_path;
    > + char name_buffer[20];
    > + u64 mac_addr_base;
    > + struct device_node *aliases;
    > + struct device_node *pip;
    > + struct device_node *iface;
    > + struct device_node *eth;
    > + struct device_node *node;
    > +
    > + /*
    > + * Edit the device tree to reflect known board
    > + * configurations.
    > + */
    > + mac_addr_base =
    > + ((octeon_bootinfo->mac_addr_base[0] & 0xffull)) << 40 |
    > + ((octeon_bootinfo->mac_addr_base[1] & 0xffull)) << 32 |
    > + ((octeon_bootinfo->mac_addr_base[2] & 0xffull)) << 24 |
    > + ((octeon_bootinfo->mac_addr_base[3] & 0xffull)) << 16 |
    > + ((octeon_bootinfo->mac_addr_base[4] & 0xffull)) << 8 |
    > + (octeon_bootinfo->mac_addr_base[5] & 0xffull);
    > +
    > + aliases = of_find_node_by_path("/aliases");
    > + if (!aliases) {
    > + pr_err("Error: No /aliases node in device tree.");
    > + return -EINVAL;
    > + }
    > +
    > + 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) {

    Ditto here to comment above.

    > + mgmt = of_find_node_by_path(node_path);
    > + if (!mgmt)
    > + continue;
    > + /* Set the ethernet address */
    > + rv = set_mac_addr_prop(mgmt, mac_addr_base);
    > + if (rv)
    > + goto err;
    > + mac_addr_base++;
    > +
    > + 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++) {
    > + iface = octeon_of_get_child(pip, i);
    > + if (!iface)
    > + continue;
    > + for (p = 0; p < 4; p++) {
    > + int phy = cvmx_helper_board_get_mii_address(16 * i + p);
    > + eth = octeon_of_get_child(iface, p);
    > + if (!eth)
    > + continue;
    > + node = of_parse_phandle(eth, "phy-handle", 0);
    > + rv = set_mac_addr_prop(eth, mac_addr_base);
    > + mac_addr_base++;
    > + if (node && phy >= 0)
    > + set_phy_addr_prop(node, cvmx_helper_board_get_mii_address(16 * i + p));
    > + of_node_put(node);
    > + of_node_put(eth);
    > + }
    > + of_node_put(iface);
    > + }
    > + of_node_put(pip);
    > + }
    > +
    > + of_node_put(aliases);
    > + return 0;
    > +err:
    > + return rv;
    > +}
    > +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?

    > +
    > +static int __init octeon_publish_devices(void)
    > +{
    > + return of_platform_bus_probe(NULL, octeon_ids, NULL);
    > +}
    > +device_initcall(octeon_publish_devices);
    > +
    > +
    > MODULE_AUTHOR("David Daney <ddaney@caviumnetworks.com>");
    > MODULE_LICENSE("GPL");
    > MODULE_DESCRIPTION("Platform driver for Octeon SOC");
    > diff --git a/arch/mips/cavium-octeon/setup.c b/arch/mips/cavium-octeon/setup.c
    > index ab1a106..818f66d5 100644
    > --- a/arch/mips/cavium-octeon/setup.c
    > +++ b/arch/mips/cavium-octeon/setup.c
    > @@ -20,6 +20,7 @@
    > #include <linux/platform_device.h>
    > #include <linux/serial_core.h>
    > #include <linux/serial_8250.h>
    > +#include <linux/of_fdt.h>
    > #ifdef CONFIG_BLK_DEV_INITRD
    > #include <linux/initrd.h>
    > #endif
    > @@ -797,3 +798,19 @@ void prom_free_prom_memory(void)
    > }
    > #endif
    > }
    > +
    > +int octeon_prune_device_tree(void);
    > +
    > +extern const char __dtb_octeon_3xxx_begin;
    > +extern const char __dtb_octeon_3xxx_end;
    > +void __init device_tree_init(void)
    > +{
    > + int dt_size = &__dtb_octeon_3xxx_end - &__dtb_octeon_3xxx_begin;
    > + /* Copy the default tree from init memory. */
    > + initial_boot_params = early_init_dt_alloc_memory_arch(dt_size, 8);
    > + if (initial_boot_params == NULL)
    > + panic("Could not allocate initial_boot_params\n");
    > + memcpy(initial_boot_params, &__dtb_octeon_3xxx_begin, dt_size);
    > + unflatten_device_tree();
    > + octeon_prune_device_tree();
    > +}
    > --
    > 1.7.2.3
    >


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