On Sun, 2011-07-03 at 14:51 -0600, Grant Likely wrote:

On Sat, Jul 2, 2011 at 3:15 PM, Jonas Bonn [off-list ref] wrote:
Does it really make sense to have an SoC node for this board?  I
assume this is for a soft CPU system on a Diligent FPGA board.  It is
best if the device tree structure matches the actual bus layout of the
chip and/or FPGA design.  If the devices are directly on the CPU bus,
then it is better to do without an soc node entirely and just put
everything at the root.

Not sure I quite understand the distinction here.  Yes, it's a soft CPU,
but conceptually it has an internal bus (Wishbone) comparable to the
Avalon bus that the peripherals sit on.  I'd say it's an SoC; but I need
to ask: what constitutes a valid use of the "soc" node?

quoted

diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c
index 49342e9..6ce6583 100644
--- a/arch/openrisc/kernel/setup.c
+++ b/arch/openrisc/kernel/setup.c

@@ -173,8 +173,7 @@ void __init setup_cpuinfo(void)
       unsigned long iccfgr,dccfgr;
       unsigned long cache_set_size, cache_ways;;

-       cpu = (struct device_node *) of_find_compatible_node(NULL,
-                                       NULL, "opencores,openrisc-1200");
+       cpu = of_find_compatible_node(NULL, NULL, "opencores,openrisc-1200");

This looks odd.  Is it a stale hunk from a previous patch version?

Hmmm... no, it's a hunk that should have been in patch 01/19 of the
series.  That said, there's a bit of devicetree code in that patch too.
Can I just say, "please have a look there, too", or do I need to pull
those bits out into the devicetree patch?

I've pasted in just the relevant parts from patch 01/19 below for ease
of review.

/Jonas

From arch/openrisc/kernel/setup.c:

+static inline unsigned int fcpu(struct device_node *cpu, char *n)
+{
+        const int *val;
+        return (val = of_get_property(cpu, n, NULL)) ? *val : 0;
+}
+
+void __init setup_cpuinfo(void)
+{
+       struct device_node *cpu = NULL;
+       unsigned long iccfgr,dccfgr;
+       unsigned long cache_set_size, cache_ways;;
+
+       cpu = (struct device_node *) of_find_compatible_node(NULL,
+                                       NULL,
"opencores,openrisc-1200");
+       if (!cpu) {
+               panic("No compatible CPU found in device tree...\n");
+       }
+
+       iccfgr = mfspr(SPR_ICCFGR);
+       cache_ways = 1 << (iccfgr & SPR_ICCFGR_NCW);
+       cache_set_size = 1 << ((iccfgr & SPR_ICCFGR_NCS) >> 3);
+       cpuinfo.icache_block_size = 16 << ((iccfgr & SPR_ICCFGR_CBS) >>
7);
+       cpuinfo.icache_size = cache_set_size * cache_ways *
cpuinfo.icache_block_size;
+
+       dccfgr = mfspr(SPR_DCCFGR);
+       cache_ways = 1 << (dccfgr & SPR_DCCFGR_NCW);
+       cache_set_size = 1 << ((dccfgr & SPR_DCCFGR_NCS) >> 3);
+       cpuinfo.dcache_block_size = 16 << ((dccfgr & SPR_DCCFGR_CBS) >>
7);
+       cpuinfo.dcache_size = cache_set_size * cache_ways *
cpuinfo.dcache_block_size;
+
+       cpuinfo.clock_frequency =  fcpu(cpu, "clock-frequency");
+
+       of_node_put(cpu);
+
+       print_cpuinfo();
+}
+
+/**
+ * or32_early_setup
+ *
+ * Handles the pointer to the device tree that this kernel is to use
+ * for establishing the available platform devices.
+ *
+ * For now, this is limited to using the built-in device tree.  In the
future,
+ * it is intended that this function will take a pointer to the device
tree
+ * that is potentially built-in, but potentially also passed in by the
+ * bootloader, or discovered by some equally clever means...
+ */
+
+void __init or32_early_setup(void) {
+
+       early_init_devtree((void *) __dtb_start);
+
+       printk(KERN_INFO "Compiled-in FDT at 0x%08x\n",
+              (unsigned int) __dtb_start);
+}
+
+const struct of_device_id openrisc_bus_ids[] = {
+        { .type = "soc", },
+        { .compatible = "soc", },
+        {},
+};
+
+static int __init openrisc_device_probe(void)
+{
+        of_platform_bus_probe(NULL, openrisc_bus_ids, NULL);
+        return 0;
+}
+device_initcall(openrisc_device_probe);