Re: [RFC PATCH 06/10] MIPS: Octeon: Initialize and fixup devicetree.

[Grant Likely]

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, 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@xxxxxxxxxxxxxxxxxx>");
>  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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/